Merge pull request #16709 from rabbitmq/address-v-sec-issues

Refactor HTTP API responses

Author: michaelklishin

deps/oauth2_client/src/jwt_helper.erl

@@ -18,5 +18,6 @@ decode(Token) ->
         {error, {invalid_token, Type, Err, Stacktrace}}
     end.
 
-get_expiration_time(#{<<"exp">> := Exp}) when is_integer(Exp) -> {ok, Exp};
+get_expiration_time(#{<<"exp">> := Exp}) when is_number(Exp) -> {ok, trunc(Exp)};
+get_expiration_time(#{<<"exp">> := _}) -> {error, invalid_exp_field};
 get_expiration_time(#{}) -> {error, missing_exp_field}.

deps/rabbit/src/rabbit_auth_backend_internal.erl

@@ -137,9 +137,26 @@ internal_check_user_login(Username, Fun) ->
                 _    -> Refused
             end;
         {error, not_found} ->
+            %% Equalise timing regardless of whether the username exists to
+            %% prevent username enumeration via a timing side-channel.
+            %% The result is always discarded.
+            _ = Fun(dummy_sentinel_user()),
             Refused
     end.
 
+%% A sentinel internal_user whose password hash will never match any real
+%% cleartext. Used solely to perform a dummy hash computation on the
+%% not-found path, equalising timing with the found-but-wrong-password path.
+%% Layout: 4 bytes zeroed salt + 32 bytes zeroed SHA-256 payload.
+-define(DUMMY_PASSWORD_HASH, <<0:32, 0:256>>).
+
+dummy_sentinel_user() ->
+    HashingMod = rabbit_password:hashing_mod(),
+    internal_user:set_password_hash(
+        internal_user:new({hashing_algorithm, HashingMod}),
+        ?DUMMY_PASSWORD_HASH,
+        HashingMod).
+
 check_vhost_access(#auth_user{username = Username}, VHostPath, _AuthzData) ->
     rabbit_db_user:get_user_permissions(Username, VHostPath) =/= undefined.
 

deps/rabbit/test/unit_access_control_SUITE.erl

@@ -32,6 +32,7 @@ groups() ->
       {sequential_tests, [], [
           login_with_credentials_but_no_password,
           login_of_passwordless_user,
+          login_of_nonexistent_user,
           set_tags_for_passwordless_user,
           change_password,
           auth_backend_internal_expand_topic_permission
@@ -188,6 +189,26 @@ login_of_passwordless_user1(_Config) ->
 
     passed.
 
+login_of_nonexistent_user(Config) ->
+    passed = rabbit_ct_broker_helpers:rpc(Config, 0,
+      ?MODULE, login_of_nonexistent_user1, [Config]).
+
+login_of_nonexistent_user1(_Config) ->
+    Username = <<"login_of_nonexistent_user-user">>,
+    Password = <<"login_of_nonexistent_user-password">>,
+    %% Ensure the user does not exist before the test.
+    case rabbit_auth_backend_internal:lookup_user(Username) of
+        {ok, _} -> rabbit_auth_backend_internal:delete_user(Username, <<"acting-user">>);
+        _       -> ok
+    end,
+    %% Authentication of a non-existent user must return {refused, ...} and
+    %% must not crash or return an error tuple.
+    ?assertMatch(
+       {refused, _Message, [Username]},
+       rabbit_auth_backend_internal:user_login_authentication(
+           Username, [{password, Password}])),
+    passed.
+
 
 set_tags_for_passwordless_user(Config) ->
     passed = rabbit_ct_broker_helpers:rpc(Config, 0,

deps/rabbitmq_auth_backend_oauth2/src/rabbit_auth_backend_oauth2.erl

@@ -160,7 +160,6 @@ update_state(AuthUser, NewToken) ->
 -spec expiry_timestamp(rabbit_types:auth_user()) -> integer() | never.
 expiry_timestamp(#auth_user{impl = DecodedTokenFun}) ->
     case DecodedTokenFun() of
-        %% "exp" may be a fractional NumericDate (RFC 7519); accept floats too.
         #{<<"exp">> := Exp} when is_number(Exp) ->
             trunc(Exp);
         _ ->
@@ -236,14 +235,15 @@ ensure_same_username(PreferredUsernameClaims, CurrentDecodedToken, NewDecodedTok
         _ -> {error, mismatch_username_after_token_refresh}
     end.
 
-%% "exp" may be a fractional NumericDate (RFC 7519); accept floats too.
 validate_token_expiry(#{<<"exp">> := Exp}) when is_number(Exp) ->
-    ExpSeconds = trunc(Exp),
     Now = os:system_time(seconds),
-    case ExpSeconds =< Now of
-        true  -> {error, rabbit_misc:format("Provided JWT token has expired at timestamp ~tp (validated at ~tp)", [ExpSeconds, Now])};
+    ExpTs = trunc(Exp),
+    case ExpTs =< Now of
+        true  -> {error, rabbit_misc:format("Provided JWT token has expired at timestamp ~tp (validated at ~tp)", [ExpTs, Now])};
         false -> ok
     end;
+validate_token_expiry(#{<<"exp">> := _}) ->
+    {error, "Provided JWT token has an invalid exp claim: value is not a number"};
 validate_token_expiry(#{}) -> ok.
 
 -spec check_token(raw_jwt_token(), {resource_server(), internal_oauth_provider()}) ->

deps/rabbitmq_auth_backend_oauth2/test/unit_SUITE.erl

@@ -40,7 +40,8 @@ all() ->
         test_restricted_vhost_access_with_a_valid_token,
         test_insufficient_permissions_in_a_valid_token,
         test_token_expiration,
-        test_token_expiration_with_float_exp,
+        test_token_expiry_with_float_exp,
+        test_token_expiry_with_non_numeric_exp,
         test_invalid_signature,
         test_incorrect_kid,
         normalize_token_scope_using_multiple_scopes_key,
@@ -1242,6 +1243,40 @@ test_token_expiration(_) ->
     ?assertMatch({refused, _, _},
                  user_login_authentication(Username, [{password, Token}])).
 
+test_token_expiry_with_float_exp(_) ->
+    Username = <<"username">>,
+    set_env(resource_server_id, <<"rabbitmq">>),
+
+    %% A valid, future float exp must be accepted and expiry_timestamp must
+    %% return the truncated integer — not the atom 'never'.
+    FutureFloatExp = float(os:system_time(seconds) + 600),
+    FutureToken = (?UTIL_MOD:token_with_sub(?UTIL_MOD:expirable_token(), Username))
+                    #{<<"exp">> := FutureFloatExp},
+    {ok, #auth_user{username = Username} = User} =
+        user_login_authentication(Username, [{password, FutureToken}]),
+    ?assertEqual(trunc(FutureFloatExp), rabbit_auth_backend_oauth2:expiry_timestamp(User)),
+
+    %% An already-expired float exp must be refused, not silently accepted.
+    PastFloatExp = float(os:system_time(seconds) - 10),
+    ExpiredToken = (?UTIL_MOD:token_with_sub(?UTIL_MOD:expirable_token(), Username))
+                    #{<<"exp">> := PastFloatExp},
+    ?assertMatch({refused, _, _},
+                 user_login_authentication(Username, [{password, ExpiredToken}])).
+
+test_token_expiry_with_non_numeric_exp(_) ->
+    Username = <<"username">>,
+    set_env(resource_server_id, <<"rabbitmq">>),
+
+    %% A token whose exp claim is not a number (e.g. a string) must be refused,
+    %% not silently accepted because the is_number guard falls through to the
+    %% no-exp-field catch-all clause.
+    lists:foreach(fun(NonNumericExp) ->
+        InvalidToken = (?UTIL_MOD:token_with_sub(?UTIL_MOD:expirable_token(), Username))
+                        #{<<"exp">> := NonNumericExp},
+        ?assertMatch({refused, _, _},
+                     user_login_authentication(Username, [{password, InvalidToken}]))
+    end, [<<"1700000300">>, true, false, null]).
+
 test_incorrect_kid(_) ->
     AltKid   = <<"other-token-key">>,
     Username = <<"username">>,

deps/rabbitmq_management/priv/www/js/dispatcher.js

@@ -308,11 +308,13 @@ dispatcher_add(function(sammy) {
         });
       let datamodel = {
         'limits': '/vhost-limits',
-        'user_limits': '/user-limits',
         'vhosts': '/vhosts'
       }
       if (ac.isAdministratorUser()) {
+        datamodel['user_limits'] = '/user-limits'
         datamodel['users'] = '/users'
+      } else {
+        datamodel['user_limits'] = '/user-limits/' + esc(user_name)
       }
       path('#/limits', datamodel, 'limits');
 

deps/rabbitmq_management/src/rabbit_mgmt_login.erl

@@ -123,7 +123,7 @@ handleAccessToken(Req0, AccessToken, State) ->
 redirect_to_home_with_cookie(CookieName, CookieValue, Req=#{scheme := Scheme}, State) ->
     CookieSettings0 = #{
         http_only => true,
-        path => rabbit_mgmt_util:get_oauth2_bootstrap_cookie_path(),
+        path => rabbit_mgmt_util:prefixed_path(?OAUTH2_BOOTSTRAP_PATH),
         max_age => 30,
         same_site => strict
     },

deps/rabbitmq_management/src/rabbit_mgmt_oauth_bootstrap.erl

@@ -76,7 +76,8 @@ get_auth_mechanism(Req) ->
                                     <<"">>, Req, #{
                                         max_age => 0,
                                         http_only => true,
-                                        path => rabbit_mgmt_util:get_oauth2_bootstrap_cookie_path(),
+                                        path => iolist_to_binary([rabbit_mgmt_util:get_path_prefix(),
+                                                                  ?OAUTH2_BOOTSTRAP_PATH]),
                                         same_site => strict
                                 }),
                                 Auth
@@ -128,7 +129,7 @@ set_token_auth(AuthSettings, Req0) ->
                                     ?OAUTH2_ACCESS_TOKEN, <<"">>, Req0, #{
                                         max_age => 0,
                                         http_only => true,
-                                        path => rabbit_mgmt_util:get_oauth2_bootstrap_cookie_path(),
+                                        path => rabbit_mgmt_util:prefixed_path(?OAUTH2_BOOTSTRAP_PATH),
                                         same_site => strict
                                     }),
                                 ["set_token_auth(", rabbit_json:encode(Token), ");"]

deps/rabbitmq_management/src/rabbit_mgmt_util.erl

@@ -46,7 +46,7 @@
 -export([direct_request/6]).
 -export([qs_val/2]).
 -export([get_path_prefix/0]).
--export([get_oauth2_bootstrap_cookie_path/0]).
+-export([prefixed_path/1]).
 -export([catch_no_such_user_or_vhost/2]).
 -export([method_not_allowed/3]).
 
@@ -467,10 +467,11 @@ fixup_prefix(EnvPrefix) when is_list(EnvPrefix) ->
 fixup_prefix(EnvPrefix) when is_binary(EnvPrefix) ->
     fixup_prefix(rabbit_data_coercion:to_list(EnvPrefix)).
 
-%% Path for the short-lived /login cookies. It must carry the HTTP API path
-%% prefix, or the browser will not send them to the prefixed bootstrap.js.
-get_oauth2_bootstrap_cookie_path() ->
-    iolist_to_binary([get_path_prefix(), ?OAUTH2_BOOTSTRAP_PATH]).
+%% Prepends the configured path prefix to Path, returning a binary.
+%% Used wherever a cookie or redirect path must reflect the management
+%% plugin's configured path_prefix.
+prefixed_path(Path) ->
+    iolist_to_binary([get_path_prefix(), Path]).
 
 %% XXX sort_list_and_paginate/2 is a more proper name for this function, keeping it
 %% with this name for backwards compatibility

deps/rabbitmq_management/src/rabbit_mgmt_wm_user_limits.erl

@@ -24,7 +24,14 @@ allowed_methods(ReqData, Context) ->
     {[<<"GET">>, <<"OPTIONS">>], ReqData, Context}.
 
 is_authorized(ReqData, Context) ->
-    rabbit_mgmt_util:is_authorized_vhost_visible(ReqData, Context).
+    case user(ReqData) of
+        none ->
+            %% listing all users' limits is an administrator-only operation
+            rabbit_mgmt_util:is_authorized_admin(ReqData, Context);
+        Username ->
+            %% reading a specific user's limits: administrator/monitor or own account
+            rabbit_mgmt_util:is_authorized_user(ReqData, Context, [{user, Username}])
+    end.
 
 content_types_provided(ReqData, Context) ->
    {[{<<"application/json">>, to_json}], ReqData, Context}.