Skip to content

Add defense-in-depth against mass assignment in authn/z controllers

Nick Malcolm requested to merge mass-assignment-defense-in-depth into master

What does this MR do and why?

Add defense-in-depth against mass assignment in authn/z controllers

In the future, we might make a change to how we handle user request parameters in a way that has unexpected and undesired consequence; specifically mass assignment vulnerabilities. (There are currently none known). These additional unit tests and/or explicit type-casts are intended to defend against that future scenario.

For example: attempting to brute force a password by sending many passwords in a single request for a single user should never work. Nor should sending multiple OTP codes. The reason they might inadvertently work is because Ruby / Rails often doesn't mind if you send a string or an array of strings. For example:

# POST /vulnerable?email=fake@attacker.com
> User.find_by(email: params[:email])
# User Load (3.4ms)  SELECT "users".* FROM "users" WHERE "users"."email" = 'fake@attacker.com' LIMIT 1
=> nil

# We expect email to be a string, but what if it's not?
# POST /vulnerable?email[]=fake@attacker.com&email[]=admin@example.com
> User.find_by(email: params[:email])
# User Load (1.6ms)  SELECT "users".* FROM "users" WHERE "users"."email" IN ('fake@attacker.com', 'admin@example.com')
=> #<User id:1 @root>

This work resolves https://gitlab.com/gitlab-org/gitlab/-/issues/442831+

Methodology

The methodology was to look at authentication & authorization-related controllers, and down into any Helpers/Services/etc that are called or included.

Controller Notes
app/controllers/ldap/omniauth_callbacks_controller.rb Mainly gem code. No params to mass assign.
app/controllers/oauth/applications_controller.rb Includes OauthApplications which looks to use strong params correctly 👍 This is about creating OAuth apps anyway, not authenticating/authorizing with them.
app/controllers/oauth/authorizations_controller.rb No issues here. Added a .to_s anyway.
app/controllers/oauth/authorized_applications_controller.rb Not authn/z related. Skipped
app/controllers/oauth/token_info_controller.rb No params to mass assign.
app/controllers/oauth/tokens_controller.rb ''
app/controllers/application_controller.rb This calls via SessionlessAuthentication to all the AuthFinders. Updated those1. Otherwise fine
app/controllers/invites_controller.rb One param cast .to_s
app/controllers/jwt_controller.rb Calls some services, added a .to_s to DependencyProxyAuthenticationService
app/controllers/omniauth_callbacks_controller.rb Calls ValidateManualOtpService which I'll skip2. Otherwise no params to mass assign.
app/controllers/passwords_controller.rb resource_from_email is a fairly old method, but used in the ee class so I added a .to_s.
app/controllers/registrations_controller.rb destroy_confirmation_valid? calls current_user.valid_password?(params[:password]) so I went and added a spec that validate passing a param to valid_password? raises an error. onboarding_status_params calling params.to_unsafe_h looks smelly but not related to authn/z.
ee/app/controllers/ee/ldap/omniauth_callbacks_controller.rb No params to mass assign.
ee/app/controllers/ee/passwords_controller.rb Added a to_s to a params being passed in an audit log
ee/app/controllers/ee/registrations_controller.rb Via Arkose::TokenVerifiable I added a params[:arkose_labs_token].to_s.
ee/app/controllers/ee/sessions_controller.rb One instance updating to ::User.find_by_login(user_params[:login].to_s)
ee/app/controllers/oauth/geo_auth_controller.rb 🤷 This one seemed complicated to add casting, and the specs indicate it's a downstream that'd need to parse & reject an array of tokens.
ee/app/controllers/phone_verification/telesign_callbacks_controller.rb Looks like it could do with some protection (PhoneNumberValidation#by_reference_id would take an array), but it can only be called with a digest based on a shared secret. Will leave it alone.
ee/app/controllers/users/base_identity_verification_controller.rb phone_verification_params is Strong Params 👍 Adding specs seemed a bit unweildy: there isn't a controller spec to add to, and the requests specs stubs out the stuff we'd want to test, and then changing VerifyCodeService seems to close to the metal to mess with without more context on if casting .to_s will break something. I think if Arkose accepts multiple token attempts that's their bug. (Plus were not sending them multiple anyway, thanks to strong params)
ee/app/controllers/users/registrations_identity_verification_controller.rb Fine
ee/app/controllers/omniauth_kerberos_controller.rb No params here or in the KerberosHelpers.
ee/app/controllers/smartcard_controller.rb Seemed fine.
  1. find_user_from_static_object_token, find_user_from_feed_token, find_user_from_job_token, all raised variations on NoMethodError or TypeError. Changing to .to_s as defense in depth was done anyway.
  2. ValidateManualOtpService skipped because:
    • Passing an array to Devise TwoFactorAuthenticatable already throws an error when it tries to gsub out spaces in the OTP
    • I'm not confident to cast to_s for the implementation of FortiAuthenticator, FortiTokenCloud, and DuoAuth.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Merge request reports