6 ms·

This doesn't look like a security issue, unless I'm missing something.

by boomskats 5y ago

pavon 5y ago
Indeed the MR template does not have the security box checked.
- kinix 5y ago
  My guess is that the username is used in a url somewhere? So browsers might try and interpret it as a file
  - numlock86 5y ago
    That's what actual MIME types are for. You can serve a .jpg as a .txt file and still have the browser display it correctly as an image if you use image/jpeg. Same for the other way around or any other combination of file and MIME types.
    - kinix 5y ago
      You're not wrong, but I bet there are lots of things that will assume based on url (rightly or wrongly). e.g. I've seen a lot of nginx reverse proxy setups implement different caching policies based on url suffix (again rightly or wrongly). Maybe just playing it safe?
      - numlock86 5y ago
        Yeah, if you for example use the 1001th hip electron based browser (with some hand-crafted curl workers and custom fetchers with an express based proxy-backend included in the background mangled together with some weird mix of 20% javascript and 80% typescript and a combination of at least 400 different npm packages over a commit history of two weekends) to browse the web ... of course, that assumption might break. :^)
    - jasonjayr 5y ago
      Do you remember when MSIE would ignore the MIME type the server sent, and sniffed the contents of the beginning of the file downloaded and overrode what the server sent? Good times ..... (rocking in corner ....)
LewisVerstappen 5y ago
Here's a link to the fix: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65954/diffs https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65954/...
- tankenmate 5y ago
  I get the sneaking suspicion that this is a case of Ruby's magic being slightly too magic; it's a problem I have tripped across in the past. Slightly tangentially reminds me of the "More Magic" switch of GLS fame.
  - LewisVerstappen 5y ago
    >Slightly tangentially reminds me of the "More Magic" switch of GLS fame. Can you elaborate on what you're referencing? Tried googling but couldn't find anything.
    - robocat 5y ago
      http://www.catb.org/jargon/html/magic-story.html http://www.catb.org/jargon/html/magic-story.html
      - mdip 5y ago
        Thanks for the link -- you solved a nagging mystery for me[0]. In the '90s, I worked for a tech guy from Texas who referred to the clippers that we used to cut the ends of zip ties off as dikes. Of course, being a teenager, the term often came with a raised eyebrow from me since it was commonly used as a slur for lesbians. He was the only person I've encountered in my life who used that term in that manner and over the years I just assumed it was some bit of obscurity related to where he started working in systems. I guess not! [0] I'm sure the right Gooble query would have gotten me an answer but it teetered on the edge of "I don't care enough to bother" until the answer was presented.
  - zorr 5y ago
    There is not much ruby magic in that code. This is just a naive use of data from another module. Somewhere in the Gitlab code base there is a MIME_TYPES map with common extensions as the map key. No idea what it is used for but that module is very likely the target of a recent security issue. The first fix to combat the "publicly unknown" vulnerability was to prevent usernames ending with any of the keys in the MIME_TYPES map using a simple "ends_with" strings check. Of course the map keys did not have periods so the ends_with would also match "Asimov" with the "mov" suffix. The second fix in this PR is to extend the ends_with check to add an extra dot. The actual vulnerability is still unknown but I suspect it's something like an intermediate component that performs special handling based on interpreting URLs and that could bypass security/ACL checks.
nine_k 5y ago
It could maybe potentially be exploited in a very interestingly crafted email, where there's link to download something (e.g. the source tarball, or a build artifact) with an URL containing the username, or being otherwise close by, so that the downloaded file would be interpreted differently. But I'm not creative enough at this hour to suggest a working exploit.
- foota 5y ago
  I've played with this before. A correctly implemented mail library should handle e.g., subject lines that contain SMTP control characters. I developed a lengthy repro for an email parsing issue in an ancient version of some java email library that contained a truely horrendous parser, only to find out that the library had been updated internally recently :-)
- a-dub 5y ago
  MIME types are used all over the place: 1) web servers, browsers, proxies 2) graphical os shells 3) email every file a webserver returns has a mime type in the header, and that is how the browser knows how to present it.
  - dolmen 5y ago
    The ticket here reveals some blacklisting of known file extensions (attached to a know list of MIME type), not MIME types directly.
  - mmis1000 5y ago
    Even webserver returns a mime, browser may sometimes still ignore it and guess it themselves, because server may be wrong. Unless the server ask them `absolutely don't do it` via the header. a.k.a. `X-Content-Type-Options: nosniff` But even there is a header for that, there will still be situations that clients don't care and guess it themselves (probably because software is too old or other reason).
    - amenghra 5y ago
      Correct. You want to set Content-Type and X-Content-Type-Options. The story is that in some cases, Internet Explorer could be tricked to ignore the Content-Type header. Instead of fixing the core bug, Microsoft decided to add a header (presumably, they didn’t want to break existing websites). A decade later, people discovered two ways to bypass X-Content-Type-Options. This time, Microsoft fixed the core issue instead of adding X-Content-Type-Options-For-Reals.
paxys 5y ago
Definitely a security issue. - The merge request which originally added this check is inaccessible (https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1450 https://gitlab.com/gitlab-org/security/gitlab/-/merge_reques...) - In the issue comments the Gitlab employee says "Sorry, I cannot go into details right now. I will link the issue here once it goes public, is it ok?"

This doesn't look like a security issue, unless I'm missing something.

pavon 5y ago

Indeed the MR template does not have the security box checked.

kinix 5y ago
My guess is that the username is used in a url somewhere? So browsers might try and interpret it as a file
- numlock86 5y ago
  That's what actual MIME types are for. You can serve a .jpg as a .txt file and still have the browser display it correctly as an image if you use image/jpeg. Same for the other way around or any other combination of file and MIME types.
  - kinix 5y ago
    You're not wrong, but I bet there are lots of things that will assume based on url (rightly or wrongly). e.g. I've seen a lot of nginx reverse proxy setups implement different caching policies based on url suffix (again rightly or wrongly). Maybe just playing it safe?
    - numlock86 5y ago
      Yeah, if you for example use the 1001th hip electron based browser (with some hand-crafted curl workers and custom fetchers with an express based proxy-backend included in the background mangled together with some weird mix of 20% javascript and 80% typescript and a combination of at least 400 different npm packages over a commit history of two weekends) to browse the web ... of course, that assumption might break. :^)
  - jasonjayr 5y ago
    Do you remember when MSIE would ignore the MIME type the server sent, and sniffed the contents of the beginning of the file downloaded and overrode what the server sent? Good times ..... (rocking in corner ....)

LewisVerstappen 5y ago

Here's a link to the fix: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65954/diffs https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65954/...

tankenmate 5y ago
I get the sneaking suspicion that this is a case of Ruby's magic being slightly too magic; it's a problem I have tripped across in the past. Slightly tangentially reminds me of the "More Magic" switch of GLS fame.
- LewisVerstappen 5y ago
  >Slightly tangentially reminds me of the "More Magic" switch of GLS fame. Can you elaborate on what you're referencing? Tried googling but couldn't find anything.
  - robocat 5y ago
    http://www.catb.org/jargon/html/magic-story.html http://www.catb.org/jargon/html/magic-story.html
    - mdip 5y ago
      Thanks for the link -- you solved a nagging mystery for me[0]. In the '90s, I worked for a tech guy from Texas who referred to the clippers that we used to cut the ends of zip ties off as dikes. Of course, being a teenager, the term often came with a raised eyebrow from me since it was commonly used as a slur for lesbians. He was the only person I've encountered in my life who used that term in that manner and over the years I just assumed it was some bit of obscurity related to where he started working in systems. I guess not! [0] I'm sure the right Gooble query would have gotten me an answer but it teetered on the edge of "I don't care enough to bother" until the answer was presented.
- zorr 5y ago
  There is not much ruby magic in that code. This is just a naive use of data from another module. Somewhere in the Gitlab code base there is a MIME_TYPES map with common extensions as the map key. No idea what it is used for but that module is very likely the target of a recent security issue. The first fix to combat the "publicly unknown" vulnerability was to prevent usernames ending with any of the keys in the MIME_TYPES map using a simple "ends_with" strings check. Of course the map keys did not have periods so the ends_with would also match "Asimov" with the "mov" suffix. The second fix in this PR is to extend the ends_with check to add an extra dot. The actual vulnerability is still unknown but I suspect it's something like an intermediate component that performs special handling based on interpreting URLs and that could bypass security/ACL checks.

nine_k 5y ago

It could maybe potentially be exploited in a very interestingly crafted email, where there's link to download something (e.g. the source tarball, or a build artifact) with an URL containing the username, or being otherwise close by, so that the downloaded file would be interpreted differently. But I'm not creative enough at this hour to suggest a working exploit.

foota 5y ago
I've played with this before. A correctly implemented mail library should handle e.g., subject lines that contain SMTP control characters. I developed a lengthy repro for an email parsing issue in an ancient version of some java email library that contained a truely horrendous parser, only to find out that the library had been updated internally recently :-)
a-dub 5y ago
MIME types are used all over the place: 1) web servers, browsers, proxies 2) graphical os shells 3) email every file a webserver returns has a mime type in the header, and that is how the browser knows how to present it.
- dolmen 5y ago
  The ticket here reveals some blacklisting of known file extensions (attached to a know list of MIME type), not MIME types directly.
- mmis1000 5y ago
  Even webserver returns a mime, browser may sometimes still ignore it and guess it themselves, because server may be wrong. Unless the server ask them `absolutely don't do it` via the header. a.k.a. `X-Content-Type-Options: nosniff` But even there is a header for that, there will still be situations that clients don't care and guess it themselves (probably because software is too old or other reason).
  - amenghra 5y ago
    Correct. You want to set Content-Type and X-Content-Type-Options. The story is that in some cases, Internet Explorer could be tricked to ignore the Content-Type header. Instead of fixing the core bug, Microsoft decided to add a header (presumably, they didn’t want to break existing websites). A decade later, people discovered two ways to bypass X-Content-Type-Options. This time, Microsoft fixed the core issue instead of adding X-Content-Type-Options-For-Reals.

paxys 5y ago

Definitely a security issue. - The merge request which originally added this check is inaccessible (https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1450 https://gitlab.com/gitlab-org/security/gitlab/-/merge_reques...) - In the issue comments the Gitlab employee says "Sorry, I cannot go into details right now. I will link the issue here once it goes public, is it ok?"