Sudo sessions no longer require a valid password to impersonate users

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
User Accounts
--
critical
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Frédéric Buclin, Assigned: Max Kanat-Alexander)

Tracking

({regression, selenium})

3.3.4
Bugzilla 3.4
regression, selenium
Bug Flags:
approval +
approval3.4 +
blocking3.4 +
testcase +

Details

Attachments

(1 attachment)

v1
1.33 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
Found by one of our Selenium test scripts (3.4 only; 3.2 is fine):

#   Failed test 'get_title, 'Invalid Username Or Password''
#   at test_sudo_sessions.t line 123.                      
#          got: 'Match Failed'                             
#     expected: 'Invalid Username Or Password'             
not ok 103 - get_title, 'Invalid Username Or Password'     

I can reproduce the problem manually: in 3.2 and older, a sudoer has to enter his password in order to impersonate a user. In 3.4, you can leave the password field blank and sudo the user anyway. This means credentials are not checked correctly.

The problem comes from Bugzilla->login(LOGIN_REQUIRED); In 3.2, it complains that you left the password field empty. In 3.4, it happily let you go through (probably because it found a valid cookie).

No idea what regressed this.
Flags: blocking3.4+
(Assignee)

Comment 1

9 years ago
The problem is that Cookie is being checked before CGI--the Stack is backwards, most likely caused by the code that implements the Stack hook.
Assignee: user-accounts → mkanat
Status: NEW → ASSIGNED
Depends on: 438435
Flags: testcase?
(Assignee)

Comment 2

9 years ago
Created attachment 373250 [details] [diff] [review]
v1

That took a LOT of figuring out, and a very simple patch to fix.

I'm glad we caught this, because it was an underlying bug. I think that maybe we should consider getting rid of that "requires a password" for sudo thing, though--it's pretty much the illusion of security, since all you actually have to do to sudo somebody is send a cookie.
Attachment #373250 - Flags: review?(LpSolit)
(In reply to comment #2)
> though--it's pretty much the illusion of security, since all you actually have
> to do to sudo somebody is send a cookie.

So, tokenize the cookie and make it expire after some time period (like other tokens).
(In reply to comment #3)
> (In reply to comment #2)
> > though--it's pretty much the illusion of security, since all you actually have
> > to do to sudo somebody is send a cookie.
> 
> So, tokenize the cookie and make it expire after some time period (like other
> tokens).

... which seems to be bug 450013
(Reporter)

Comment 5

9 years ago
Comment on attachment 373250 [details] [diff] [review]
v1

Wow, nice catch. And this indeed correctly fixes the problem. r=LpSolit
Attachment #373250 - Flags: review?(LpSolit) → review+
(Reporter)

Comment 6

9 years ago
test_sudo_session.t is our best test about credentials, I think. Not sure we need a separate one (unless someone has an idea what to test exactly and how).
Flags: testcase?
Flags: testcase+
Flags: approval3.4+
Flags: approval+
Keywords: selenium
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)
> test_sudo_session.t is our best test about credentials, I think. Not sure we
> need a separate one (unless someone has an idea what to test exactly and how).

  We need to test that auth methods are actually called in the order they're specified, that's what I was thinking.
(Assignee)

Comment 8

9 years ago
tip:

Checking in Bugzilla/Auth/Login/Stack.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Stack.pm,v  <--  Stack.pm
new revision: 1.3; previous revision: 1.2
done
cvs up -rBUGZILLA-3_4-BRChecking in Bugzilla/Auth/Verify/Stack.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/Stack.pm,v  <--  Stack.pm
new revision: 1.3; previous revision: 1.2
done

3.4:

Checking in Bugzilla/Auth/Login/Stack.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/Stack.pm,v  <--  Stack.pm
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in Bugzilla/Auth/Verify/Stack.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/Stack.pm,v  <--  Stack.pm
new revision: 1.2.2.1; previous revision: 1.2
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.