Hacker News Reader

Comment by paxys | Hacker News Reader

TL;DR for those wondering:

- There was a yet-undisclosed security vulnerability in Gitlab usernames

- Staff member made a change to disallow usernames ending with `Mime::EXTENSION_LOOKUP.keys`, which I assume is a set of recognized file extensions (hidden – https://gitlab.com/gitlab-org/security/gitlab/-/merge_reques...)

- This was overly broad since it caught a lot of common names (like "asimov") (https://gitlab.com/gitlab-org/gitlab/-/issues/335278)

- The check was updated to additionally look for a "." before the extension (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65954)

View on HN

> Staff member made a change to disallow usernames ending with `Mime::EXTENSION_LOOKUP.keys`

This is a hell of a thing to commit and pass code review in 2021 on a project like GitLab. I understand that the staff member was fixing a security issue and was probably not thinking deeply about the ramifications, but even so. How many "Falsehoods programmers believe about names" articles do we need?

Surely it would be easy to run the new rules against random-list-of-usernames-found-through-google before pushing to prod? Or perhaps the security issue was deemed so great that they needed a fix out yesterday?

I mean, they’re still not telling us what it was and it’s been fixed for a month. Must’ve been pretty big.

That's usually the exact time you want to think deeply about the ramifications

I don't want to be too harsh on the programmer, everyone makes mistakes. It's easy to see how a person focused on a security issue with user names makes a quick fix without thinking through how this will affect account creation. As programmers, we have to make like 7,000 decisions every day, you're bound to fuck up some of them. This is a pretty big one, though.

The bigger question is how this passed code review and testing.

That's a good question, but I think it's still more important to understand why a root cause analysis wasn't finished. They got the cause, but not the root cause. It's like seeing your car battery dead and just replacing the battery, without understanding whether the alternator works or whether the alternator belt is broken.

What's the security issue though? Not disambiguating filenames and user names in URLs for the site? That's a hard problem to fix after the fact, which is why we call it a Rookie Mistake.

Use unique prefixes for users, groups, projects, assets in your webapp design, kids.

If anyone ever says that software engineering is completely unlike plumbing, I will point them towards your comment. All systems, however nicely architected, are full of duct-tape solutions like this.

Are they though? What I can think of (broken upstream reverse proxies that do mime type inference by filename) would warrant a WE_USE_BROKEN_LEGACY_SHIT_UPSTREAM config flag so that it doesn't get in the way of normal users.

So I'm probably missing something and I'm really curious for the underlying vulnerability.

I have no clue about the underlying issue, but I'm guessing it's occurring on a boundary or an interplay between two systems.

Something like "the username can be part of the URL, and if the URL contains .mov, some browsers will misinterpret this and assume it's a movie file, leading to bad things™".

Or: "the username is sometimes used as a folder name, and our syncing software contains rules to exclude certain file extensions, so these folders were never synced, which lead to issues on production servers"

I'm guessing it's something along these lines. Something that you control, but not really, leading to these kind of haphazard workarounds.

Professional plumbers don't use duct tape for fixing leaks ;)

As someone who has traveled to some of the lesser traveled places on this planet, they do too! Sometimes even worse techniques are being used to fix things.

GitLab team member here. This issue provides additional context on the changes: https://gitlab.com/gitlab-org/gitlab/-/issues/26295

That seems to be a consequence of what IMO is an unfortunately common bad design: having user-controlled data like usernames as the first path component (without a prefix like ~). There are many things which are expected to be found at the root of the path (the classic example being robots.txt, but there's also favicon.ico, .well-known, and probably others; I vaguely recall that IIRC Flash used a fixed filename in the root for cross-domain access control), and you never know when a new one will be invented by someone (though .well-known is supposed to contain the spread of these "magic" names).

crossdomain.xml: https://stackoverflow.com/questions/4174317/what-is-crossdom...

Don't know the details of the vulnerability, but from the comfort of my armchair, it sounds like it's being patched in the wrong location. e.g. It's better to fix an XSS issue by escaping the input, rather than restricting the values it can take.

It doesn't represent any XSS - the details are here: https://gitlab.com/gitlab-org/gitlab/-/issues/26295

I was just using XSS as an analogy.

>- Staff member made a change to disallow usernames ending with `Mime::EXTENSION_LOOKUP.keys`, which I assume is a set of recognized file extensions (hidden – https://gitlab.com/gitlab-org/security/gitlab/-/merge_reques...)

If only github had an established system and procedure for doing code reviews before releasing security fixes...

s/hub/lab

Reply on news.ycombinator.com