Ticket #235 (new defect)

Opened 1 year ago

Last modified 7 months ago

security: mod/invite "unguessable" tokens really aren't.

Reported by: jang Assigned to: nobody
Priority: low Milestone: 0.9.3
Component: core Version: 0.9rc2
Severity: minor Keywords: security invite needs-patch
Cc: Patch Included: 0
Review Stage: accepted

Description

What's wrong with this snippet from mod/invite/lib/invite_actions.php?

$invite->code = 'i' . substr(base_convert(md5(time() . $USER->username), 16, 36), 0, 7);

Nothing, as long as you don't mind people registering for Elgg sites using unvalidated emails. This really should just be a random string of junk, not something computable by a spammer.

Change History

12/21/07 21:34:52 changed by ewout

  • keywords changed from security invite to security invite needs-patch.
  • priority changed from high to normal.
  • severity changed from critical to normal.
  • milestone changed from 0.9.0 to 1.0.

Is this something that happened to you? I would think the time() function would make the code rather difficult to compute, but I am not a security expert. I agree that a random string would be better. Please attach a patch.

(follow-up: ↓ 3 ) 12/21/07 21:49:40 changed by jang

The point about emailing someone a token is that you're trying to provide a guarantee that the account created is associated with a particular email address. Since an attacker knows what time they made the attack (in fact, the HTTP server will typically them them what time it thinks it is) and time() has a one-second resolution, this permits an attacker to validate an account registration without seeing any email, breaking the point of the token.

As to a fix: I'm not a PHP hacker, just trying to make this run under Quercus (which is where I came across this). It's not clear to me that PHP actually provides a strong source of random numbers.

(in reply to: ↑ 2 ) 12/21/07 21:52:38 changed by jang

Replying to jang:

As to a fix: I'm not a PHP hacker, just trying to make this run under Quercus (which is where I came across this). It's not clear to me that PHP actually provides a strong source of random numbers.

The PHP documentation would appear to recommend a replacement along these lines:

$invite->code = md5(uniqid(mt_rand(), true));

...munged as you see fit, I guess.

01/25/08 13:43:37 changed by ewout

  • status changed from new to assigned.
  • severity changed from normal to minor.
  • haspatch changed.
  • review_stage set to accepted.
  • priority changed from normal to low.
  • owner changed from misja to ewout.

03/13/08 11:26:31 changed by

  • milestone deleted.

Milestone 1.0 deleted

03/13/08 11:34:47 changed by misja

  • milestone set to 0.9.3.

05/13/08 16:29:55 changed by ewout

  • owner changed from ewout to nobody.
  • status changed from assigned to new.