Background information

This was originally reported as a private security issue, but has been approved for handling in the public queue by the Drupal Security Team because it falls into a category that can be fixed publicly.

Problem/Motivation

The externalauth registration flow accepts externally supplied identifier values and uses them to build the local Drupal username and authmap entry.

If those values exceed the storage limits used by the user entity or the authmap table, registration can fail with a database exception instead of being rejected cleanly before any write is attempted.

The affected code path is in ExternalAuth::register(), and similar authmap length checks are also relevant for ExternalAuth::linkExistingAccount().

Steps to reproduce

  1. Enable External Authentication and a module that uses its registration API.
  2. Attempt to register or link an account using an oversized external identifier or a username derived from it.
  3. Observe that the request can reach the database layer and fail there, instead of being rejected earlier by the module.

Proposed resolution

  • Add targeted length validation in ExternalAuth before any user or authmap write.
  • Throw ExternalAuthRegisterException when the provider, authname, username, or derived init value exceeds the supported storage limits.
  • Run the authoritative validation after the AUTHMAP_ALTER event, because subscribers may modify the final authname or username.
  • Do not truncate values, because truncation can introduce identity collisions.
  • Do not switch to full entity validation as part of this issue, because that would broaden behavior.

Remaining tasks

  • Implement targeted pre-write validation in ExternalAuth::register() and ExternalAuth::linkExistingAccount().
  • Add automated coverage for oversized provider, authname, username, and derived init values.
  • Review whether a separate follow-up issue should add optional strict entity validation, off by default.

User interface changes

None directly. Calling code may receive a cleaner exception instead of a lower-level database failure.

API changes

No new API is introduced. The existing registration and account-linking flows will reject oversized values earlier by throwing ExternalAuthRegisterException.

Data model changes

None in this issue. This change is limited to validation before existing writes.

Issue fork externalauth-3596487

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

svendecabooter created an issue. See original summary.

svendecabooter opened merge request !28

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Active » Needs review

Ready for review and feedback

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Reviewed & tested by the community

Reviewed, and tested/xdebugged with samlauth module manually to see if I really didn't overlook anything. I think this is RTBC.

From my standpoint, this doesn't change anything considerably. An exception is an exception, and should always be handled. (And is handled, in the samlauth case. The only effect is that the watchdog message becomes clearer.)

In an ideal theoretical world, I'd like to see the length checks on authname+provider tied closer to authmap->save() / moved into AuthMap instead, because that is the code that actually has the length requirement. Then again, I see how that would complicate code (because you want checks to be done before saving the user). Maybe that's for a new major version where (as you mentioned) the user entity is better validated as a whole.

So... code that uses AuthMap without the ExternalAuth service (I have seen at least one project doing this) will not get the new exception and should keep doing its own checks. But I don't know if that is a disadvantage or an advantage. (I don't have access to the security issue and don't know what was the justification for filing it. Again, this MR changes very little in practice.)

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Reviewed & tested by the community » Needs work

Oops! I'm just testing some strange database error reported by several users, and it turns out that this needs work. Commenting on the MR.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Oh, something else. I'm writing out details here, but TL/DR I don't think this is a showstopper.

In theory there can be samlauth-using projects which do custom manipulation/validation of the user name/email address in custom code, which could be blocked by your new checks. (Because this custom manipulation/validation happens after your new checks. I suspect this affects only samlauth module.)

  • It's possible that these currently live with an empty username, and populate the username from other values. So if you implement 'fix 2' (instead of 1) per my MR comment, they would suddenly get an exception. I imagine they can always work around this by changing configuration, to pass a bogus property into the username initially.
  • It's theoretically possible that these take care of shortening too long usernames and email addresses by themselves (and would suddenly get an exception). In order to do that, they would also have to have disabled samlauth's code (its standard event subscriber) that does name/email checks... I'm not sure anyone does that. If so, they just have to pin an older version of externalauth, I guess.

In case you want to know:

The reason: samlauth dispatches its own event with the not-saved-yet user account (not the authmap data) plus incoming SAML request properties, during $account->save() -> hook_user_presave().

In other words: your "Proposed resolution" text says: Run the authoritative validation after the AUTHMAP_ALTER event, because subscribers may modify the final authname or username. Totally agree, but unfortunately the samlauth module's event subscribers run even later, which IMHO you cannot reasonably account for.

hook_user_presave() is a very ugly place to do this, and I still want to fix that. But I considered the ExternalAuthRegisterEvent to be too late: if altering/validation goes wrong or the event subscriber wants to deny access, then you end up with an already-saved bogus user. (And when I wrote this code in 2017, I wasn't going to ask for a 3rd event to be added to Externalauth::register()...)

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review

Oops, wrong tab

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Needs work

Oops, wrong tab

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Needs work » Needs review

Thanks for the detailed review and for pointing out the name = NULL case.
I’ve updated the MR to handle that path cleanly as well.

Current behavior in ExternalAuth::register() is now:

  • if account_data['name'] is explicitly provided, it must be a non-empty string
  • NULL / non-string values now throw ExternalAuthRegisterException('The username must be a string.')
  • '' now throws ExternalAuthRegisterException('The username cannot be empty.')
  • the fallback to "$provider_$authname" now only happens when name was not provided at all

I also added an inline comment near that logic to make the behavior explicit, per your suggestion.
So this should avoid the lower-level DB failure / type error and fail earlier with a clearer exception instead.

I realize this may be relevant for Samlauth if it sometimes passes name => NULL, so I’d appreciate it if you could review whether this stricter handling causes any unexpected problem on your side. Would it just change from currently being a DB error to a fatal application error?

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

Would it just change from currently being a DB error to a fatal application error?

Correct - for current versions of samlauth. These now get the ExternalAuthRegisterException (I tested just to be sure), so that's good => RTBC.

I regard this as a bug in samlauth, so the next samlauth release will replace it by a better exception with a message like "Please check your configuration because the username should (of course) not be empty".

So we were just 'lucky' (or unlucky?) that this bug surfaced the fatal PHP error, otherwise I would not have spotted it.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • svendecabooter committed 4f4c929c on 2.0.x 
    fix: #3596487 Validate external identifier lengths before saving users...