Gitlab – Static passwords set during OmniAuth-based registration (CVE-2022-1162) • Hacker News Board

Re: Gitlab – Static passwords set during OmniAuth-based registration (CVE-2022-1162)

#21

Post by bloomburger » Sat, Apr 02, 2022, 4:55 AM UTC

nightpool wrote: post #18

theptip wrote: post #14

Earlier quoted context omitted.

Thanks for doing the issue sleuthing. This is an excruciatingly bad look. You'd have thought with all the code-owner functionality that GL has, they would lock down the `/lib/gitlab/auth/` files to require a security engineer to give additional signoff on top of a normal review. It looks like anyone at Gitlab can approve changes to the auth code (except LDAP): https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitla…

This was already required in this case, you can see in the comments that an additional AppSec review was done and the appsec team signed off on the MR above and beyond the normal code review process.

And two of those appsec reviewers are now out of office for the next two weeks…

Re: Gitlab – Static passwords set during OmniAuth-based registration (CVE-2022-1162)

#22

Post by bloomburger » Sat, Apr 02, 2022, 5:12 AM UTC

theptip wrote: post #17

fy20 wrote: post #9

Earlier quoted context omitted.

Is there a better way to catch errors like this? Looking through the PR it looks like this file was accidentally changed, I assume with a project wide search and replace. I could easily imagine myself missing this when reviewing the PR "oh it's just changing a whole bunch of specs, go ahead".

Posted some of this in a sibling comment, but there are a few ways you can address this (not all viable for Gitlab). First of all, in code review, having merge requests touching security-sensitive code (e.g. /lib/gitlab/auth) require review by a security engineer using CODEOWNERS is the obvious process gap. If you're paranoid, have security engineers review the diffs on all releases before they are shipped (in practi…

Is the vulnerability in the code change glaringly obvious to you?

Looks like two security engineers actually took a look at it: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...

Re: Gitlab – Static passwords set during OmniAuth-based registration (CVE-2022-1162)

#23

Post by orojackson » Sat, Apr 02, 2022, 7:17 AM UTC

bloomburger wrote: post #21

nightpool wrote: post #18

Earlier quoted context omitted.

This was already required in this case, you can see in the comments that an additional AppSec review was done and the appsec team signed off on the MR above and beyond the normal code review process.

And two of those appsec reviewers are now out of office for the next two weeks…

Heh, was this you [1]? Pretty much asking the same good questions you brought up in a different post.

Additionally, I see that the Senior Director of Engineering, Tim Zallmann, has left a bunch of GitLab project repos about 14 hours ago as of this writing. He was one of the folks who tried pinging [3] Mr. Coutable (he's one of the reviewers that's currently OOO). The ping is likely regarding the discovery of the security vulnerability.

[1] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...

[2] https://gitlab.com/users/timzallmann/activity

[3] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...

Re: Gitlab – Static passwords set during OmniAuth-based registration (CVE-2022-1162)

#24

Post by theptip » Sat, Apr 02, 2022, 5:57 PM UTC

bloomburger wrote: post #22

theptip wrote: post #17

Earlier quoted context omitted.

Posted some of this in a sibling comment, but there are a few ways you can address this (not all viable for Gitlab). First of all, in code review, having merge requests touching security-sensitive code (e.g. /lib/gitlab/auth) require review by a security engineer using CODEOWNERS is the obvious process gap. If you're paranoid, have security engineers review the diffs on all releases before they are shipped (in practi…

Is the vulnerability in the code change glaringly obvious to you? Looks like two security engineers actually took a look at it: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76318#...

It’s easy to spot in hindsight. I can see how it slipped through if they didn’t actually closely read the changes in each file, it’s easy to let your guard down with many-file changesets like this. (That’s a cognitive bias that should be kept in mind when doing security review, particularly if that’s your only control in place to prevent issues like this).

Fundamentally, “fix tests” changesets like this should not touch non-test code and anything touching /var/lib/gitlab would be a big code smell to me if I was reviewing.

The advantage of the code owners approach is it highlights that there were changes to the prod auth code (shows the rule that triggers in the MR I believe), which gives you an additional chance to spot issues to focus on. Seems they also have bots running checks and so they could have the bot ask for an extra review if …/auth/ was touched. Test helpers shouldn’t live next to prod code, for obvious reasons, so the “prod auth code owner” rule being hit would have been an alarm bell on this MR.

I do think it’s concerning that security review was given here, so I’m keen to see the post-mortem and see what other gaps they identify.