6 ms·

RCE via GitHub import

184 points by louislang 4y ago

staticassertion 4y ago
> Sawyer is a crazy class that converts a hash to an object whose methods are based on the hash's key: People keep adding hidden interpreters and 'exec' commands in their projects where no one expects them to be. As far as I can tell the library is just supposed to make HTTP requests, it's just a fancy client library? This needs to be the kind of thing that developers are trained on, like "don't hash passwords" and "don't build up SQL strings from untrusted input". "Don't build your own 'exec' and hide it in your library, don't reinvent object serialization". There is almost always a better way to solve a problem than by transmitting arbitrary code. Sometimes there isn't but it's rare, and in those cases it's very explicit ie: "I'm sending this database a query, I expect it to execute it".
- anamexis 4y ago
  The RCE here doesn't come from Sawyer `exec`ing anything. Sawyer builds objects from a hashmap, where the hashmap's properties can be accessed as method calls, recursively. The vulnerability here arises from the fact that you can override built-in methods like `to_s`, which in combination with the way that the Redis gem builds raw commands, can be used to send arbitrary commands to Redis.
  - mananaysiempre 4y ago
    ... And the fact that Ruby doesn’t really have fields, only methods. For example, the Python equivalent to this (morally, (obj := object()).__dict__.update(untrusted data)) would not be vulnerable. Which is not a point against Ruby, only against using this specific technique in Ruby.
  - dwohnitmok 4y ago
    Yeah... although monkey-patching, as this bug demonstrates, can get uncomfortably close to `exec`. I have a strong distaste for monkey-patching as a result since I'm only confident in its security when it's used purely statically (i.e. no user input can change how the monkey patching happens), but then that kind of robs the entire point of monkey-patching if you remove its dynamism.
    - inopinatus 4y ago
      Not sure if everyone will share my definition, but I wouldn’t call this monkey-patching. Yes, it is dynamic addition of methods at runtime, but almost everything in Ruby is runtime, including method definition in the common case. So I reserve the term for when one library intrusively (and often globally) modifies the behaviour of another, i.e. some clear sense of a control boundary being crossed, which is only slightly less technical because we can still denote clear boundaries. And in this case it looks like intentional (albeit naive) behaviour within the library in question.
  - staticassertion 4y ago
    I didn't mean to say it was 'exec'ing anything directly, I meant that people are reinventing 'exec' and hidden interpreters. Building up methods from runtime data is the problem I'm referring to.
- rtev 4y ago
  What’s wrong with hashing passwords? Hash with a slow algorithm is the best method, as far as I know
  - roblabla 4y ago
    As one random point: If you just hash the password, you're vulnerable to rainbow table attacks. So you want to salt the password, at the very least. But really, what you want to do is use a framework developed by domain experts that deals with all that mess for you. Because there's a lot of surprising complexity to storing password hashes securely. So it's better to use a well-vetted library that has eyeballs and mindshare checking that it is correct.
    - junon 4y ago
      Rainbow table attacks are significantly harder with properly hashed passwords, e.g. with bcrypt.
      - SahAssar 4y ago
        I think all bcrypt implementations implement salting per default. Same for any modern password hashing implementation.
        junon 4y ago
        That wasn't the point I was making. I was contrasting it with the (mis)use of e.g. SHA1 or worse, MD5.
      - lyu07282 4y ago
        I think that's what they are saying, bcrypt is secure because it uses a salt and multiple rounds of hashing.
  - tialaramex 4y ago
    Avoid passwords. "A secret is something you tell one other person, so I'm telling you". If possible adjust APIs to not rely on knowledge of secrets for their functioning, and then this entire problem evaporates. e.g. WebAuthn. If it's crucial that a human memorable secret (a password) is used, choose an asymmetrical Password Authenticated Key Exchange in which it's possible for the relying party to learn a value with which they can confirm that the other party knows the password, but never learn what that password is. This is really difficult to do properly, OPAQUE is the current recommendation of the IETF for this purpose. Only fall back to hashing passwords because you're obliged to for legacy reasons.
- nine_k 4y ago
  It's an easy slippery slope. We need flexibility because we can't predict all future needs, hence an interpreter. An interpreter can at worst produce a denial of service. But we also want it to access our data to be useful. Giving access to exact data items it may require is hard or impossible (see flexibility), so we give it a lump of access. Hello exfiltration. But crafting and supporting a limited interpreter is a chore. Why can't we use the perfectly good interpreter which runs our software? It's almost an RCE! For the win, we should note that input sanitation is hard and costs us CPU, and skip it. Now finally we made enough to have our system pwned.
- staticassertion 4y ago
  Sorry, I meant "don't store unhashed passwords".
dwohnitmok 4y ago
Kudos on the comment log in demonstrating the transparency of how this was dealt with in the intervening month.
lolinder 4y ago
> Normally, id should be a number. However when id is {"to_s": {"bytesize": 2, "to_s": "1234REDIS_COMMANDS" }}, we can inject additional redis commands by using bytesize to limit the previous command when it is constructed.... This class of bug cannot happen in a statically-typed language. When I deserialize something in Java, I know what it turned into. If it wasn't what I expected, I get a stacktrace, not an RCE vulnerability. I could write an interpreter to do weird stuff with my custom class and that could lead to an RCE, or I might have a library that does so. But barring severe malfeasance in a trusted library (like log4j) I'll never think I have a plain number and actually have untrusted code.
- danielheath 4y ago
  Java had a long list of this class of vulnerability, because it lets you build frameworks that lookup class by name. It's no longer common to do that in Java, because after the CVEs were reported, framework authors changed the API. This process also happened in ruby. Not familiar enough with other ecosystems to be sure but I'd bet money it's hit a fair few of them.
  - lolinder 4y ago
    I'd file that under "weird stuff you can do with the deserialized object", and static types certainly don't protect you from that (edit: or from dangerous serializer libraries). This bug isn't in that category, though: this happened because GitLab assumed they had a specific data type but didn't guarantee that this was always true. The build_command method, in turn, does something different depending on the type of data passed into it. This line in the Ruby code: id = id_for_already_imported_cache(object) Would translate to this in the Java code: Object id = idForAlreadyImportedCache(object) Which would immediately set off alarm bells in the mind of the author that they should probably double-check they've got the right type.
  - paranoidrobot 4y ago
    There's a similar set of problems with .NET and BinaryFormatter and a few other things. MS now have a security guide on using them: https://learn.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide https://learn.microsoft.com/en-us/dotnet/standard/serializat...
paulgb 4y ago
> Sawyer is a crazy class that converts a hash to an object whose methods are based on the hash's key: For those not familiar with Ruby terminology, "hash" in this context means what other languages would call a hash map, associative array, or dictionary. At first I thought it was referring to the hash of a value, and couldn't grok it.
- btown 4y ago
  For those on the Python side of things, https://box.readthedocs.io/en/development/ https://box.readthedocs.io/en/development/ does something similar and is a great utility to reach for when you're handling trusted JSON-like data and need an API-like interface. (The Django templating language does this too, albeit in a more magical way: https://docs.djangoproject.com/en/4.1/ref/templates/language/#variables https://docs.djangoproject.com/en/4.1/ref/templates/language...) Just... make sure you're aware of the issue that the OP's RCE brings up!
- 3PS 4y ago
  > "hash" in this context means what other languages would call a hash map Interesting. The only other time I've seen this term used for a hash map is in the context of Redis hashes [0], which confused me to no end when I first encountered them for the same reason. Given that Redis itself is written in C, I wonder where the term originated. [0] https://redis.io/docs/data-types/hashes/ https://redis.io/docs/data-types/hashes/
  - syrrim 4y ago
    Perl, probably.
  - danielheath 4y ago
    This got me curious, so I went and looked what it was called in a few languages. Perl: Hash Ruby: Hash PHP: HashMap Java: HashMap Haxe: Map Golang: map Swift: Dict Python: Dict C#: Dictionary Javascript: Object Redis: Hash Postgresql: hstore, json
    - 4y ago
whimsicalism 4y ago
It is interesting how far this discloser chose to go when investigating this vulnerability. They reproduced on their local Gitlab install, then proceeded to exploit the gitlab.com install itself and break one of their own gitlab repos. I am curious what the norms are here.
- bink 4y ago
  It would depend on the rules for the program. GitLab's HackerOne Rules of Engagement do not explicitly prohibit testing on gitlab.com except for attacks that are "disruptive" or access the private information of other users. I'd personally put running an RCE into that category, but it's subjective. https://hackerone.com/gitlab?type=team https://hackerone.com/gitlab?type=team
  - thayne 4y ago
    > I'd personally put running an RCE into that category, but it's subjective. I think that would depend on what code is being run. If it is something innocuous that just reports back that it was run, without any data, that doesn't seem disruptive or like it would access private information.
    - nigma1337 4y ago
      It does also seem like the reporter was extremely careful as to not accidentally access user data > After seeing the pings, I immediately turned off the replication by executing the redis command REPLICAOF no one\n\n. No information from your redis server has been replicated to mine as I used nc and I got only the ping messages.
      - whimsicalism 4y ago
        I mean they caused an internal server error on the Gitlab end. I agree they definitely weren't trying to access any user data other than their own.
stusmall 4y ago
I'm curious on the comment about a customer asking on the fix status. That comment was made before the thread was public. How did the customer know? Was it independently discovered or are they disclosing some known bugs to high value customers?
- lbotos 4y ago
  Look at the timing: https://about.gitlab.com/releases/2022/08/22/critical-security-release-gitlab-15-3-1-released/ https://about.gitlab.com/releases/2022/08/22/critical-securi... Blog post published this was patched on 8/22. Support Engineer commented on 8/25. This is on HN today because this is "30 days after H1 issue is closed make issue public" workflow.
akkartik 4y ago
Everything old is new again. https://news.ycombinator.com/item?id=5028218 https://news.ycombinator.com/item?id=5028218 https://www.sitepoint.com/anatomy-of-an-exploit-an-in-depth-look-at-the-rails-yaml-vulnerability https://www.sitepoint.com/anatomy-of-an-exploit-an-in-depth-... (https://news.ycombinator.com/item?id=5167508 https://news.ycombinator.com/item?id=5167508) https://news.ycombinator.com/item?id=5145397 https://news.ycombinator.com/item?id=5145397

184 points by louislang 4y ago

> Sawyer is a crazy class that converts a hash to an object whose methods are based on the hash's key: People keep adding hidden interpreters and 'exec' commands in their projects where no one expects them to be. As far as I can tell the library is just supposed to make HTTP requests, it's just a fancy client library? This needs to be the kind of thing that developers are trained on, like "don't hash passwords" and "don't build up SQL strings from untrusted input". "Don't build your own 'exec' and hide it in your library, don't reinvent object serialization". There is almost always a better way to solve a problem than by transmitting arbitrary code. Sometimes there isn't but it's rare, and in those cases it's very explicit ie: "I'm sending this database a query, I expect it to execute it".

anamexis 4y ago
The RCE here doesn't come from Sawyer `exec`ing anything. Sawyer builds objects from a hashmap, where the hashmap's properties can be accessed as method calls, recursively. The vulnerability here arises from the fact that you can override built-in methods like `to_s`, which in combination with the way that the Redis gem builds raw commands, can be used to send arbitrary commands to Redis.
- mananaysiempre 4y ago
  ... And the fact that Ruby doesn’t really have fields, only methods. For example, the Python equivalent to this (morally, (obj := object()).__dict__.update(untrusted data)) would not be vulnerable. Which is not a point against Ruby, only against using this specific technique in Ruby.
- dwohnitmok 4y ago
  Yeah... although monkey-patching, as this bug demonstrates, can get uncomfortably close to `exec`. I have a strong distaste for monkey-patching as a result since I'm only confident in its security when it's used purely statically (i.e. no user input can change how the monkey patching happens), but then that kind of robs the entire point of monkey-patching if you remove its dynamism.
  - inopinatus 4y ago
    Not sure if everyone will share my definition, but I wouldn’t call this monkey-patching. Yes, it is dynamic addition of methods at runtime, but almost everything in Ruby is runtime, including method definition in the common case. So I reserve the term for when one library intrusively (and often globally) modifies the behaviour of another, i.e. some clear sense of a control boundary being crossed, which is only slightly less technical because we can still denote clear boundaries. And in this case it looks like intentional (albeit naive) behaviour within the library in question.
- staticassertion 4y ago
  I didn't mean to say it was 'exec'ing anything directly, I meant that people are reinventing 'exec' and hidden interpreters. Building up methods from runtime data is the problem I'm referring to.
rtev 4y ago
What’s wrong with hashing passwords? Hash with a slow algorithm is the best method, as far as I know
- roblabla 4y ago
  As one random point: If you just hash the password, you're vulnerable to rainbow table attacks. So you want to salt the password, at the very least. But really, what you want to do is use a framework developed by domain experts that deals with all that mess for you. Because there's a lot of surprising complexity to storing password hashes securely. So it's better to use a well-vetted library that has eyeballs and mindshare checking that it is correct.
  - junon 4y ago
    Rainbow table attacks are significantly harder with properly hashed passwords, e.g. with bcrypt.
    - SahAssar 4y ago
      I think all bcrypt implementations implement salting per default. Same for any modern password hashing implementation.
      - junon 4y ago
        That wasn't the point I was making. I was contrasting it with the (mis)use of e.g. SHA1 or worse, MD5.
    - lyu07282 4y ago
      I think that's what they are saying, bcrypt is secure because it uses a salt and multiple rounds of hashing.
- tialaramex 4y ago
  Avoid passwords. "A secret is something you tell one other person, so I'm telling you". If possible adjust APIs to not rely on knowledge of secrets for their functioning, and then this entire problem evaporates. e.g. WebAuthn. If it's crucial that a human memorable secret (a password) is used, choose an asymmetrical Password Authenticated Key Exchange in which it's possible for the relying party to learn a value with which they can confirm that the other party knows the password, but never learn what that password is. This is really difficult to do properly, OPAQUE is the current recommendation of the IETF for this purpose. Only fall back to hashing passwords because you're obliged to for legacy reasons.
nine_k 4y ago
It's an easy slippery slope. We need flexibility because we can't predict all future needs, hence an interpreter. An interpreter can at worst produce a denial of service. But we also want it to access our data to be useful. Giving access to exact data items it may require is hard or impossible (see flexibility), so we give it a lump of access. Hello exfiltration. But crafting and supporting a limited interpreter is a chore. Why can't we use the perfectly good interpreter which runs our software? It's almost an RCE! For the win, we should note that input sanitation is hard and costs us CPU, and skip it. Now finally we made enough to have our system pwned.
staticassertion 4y ago
Sorry, I meant "don't store unhashed passwords".

dwohnitmok 4y ago

Kudos on the comment log in demonstrating the transparency of how this was dealt with in the intervening month.

lolinder 4y ago

> Normally, id should be a number. However when id is {"to_s": {"bytesize": 2, "to_s": "1234REDIS_COMMANDS" }}, we can inject additional redis commands by using bytesize to limit the previous command when it is constructed.... This class of bug cannot happen in a statically-typed language. When I deserialize something in Java, I know what it turned into. If it wasn't what I expected, I get a stacktrace, not an RCE vulnerability. I could write an interpreter to do weird stuff with my custom class and that could lead to an RCE, or I might have a library that does so. But barring severe malfeasance in a trusted library (like log4j) I'll never think I have a plain number and actually have untrusted code.

danielheath 4y ago
Java had a long list of this class of vulnerability, because it lets you build frameworks that lookup class by name. It's no longer common to do that in Java, because after the CVEs were reported, framework authors changed the API. This process also happened in ruby. Not familiar enough with other ecosystems to be sure but I'd bet money it's hit a fair few of them.
- lolinder 4y ago
  I'd file that under "weird stuff you can do with the deserialized object", and static types certainly don't protect you from that (edit: or from dangerous serializer libraries). This bug isn't in that category, though: this happened because GitLab assumed they had a specific data type but didn't guarantee that this was always true. The build_command method, in turn, does something different depending on the type of data passed into it. This line in the Ruby code: id = id_for_already_imported_cache(object) Would translate to this in the Java code: Object id = idForAlreadyImportedCache(object) Which would immediately set off alarm bells in the mind of the author that they should probably double-check they've got the right type.
- paranoidrobot 4y ago
  There's a similar set of problems with .NET and BinaryFormatter and a few other things. MS now have a security guide on using them: https://learn.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide https://learn.microsoft.com/en-us/dotnet/standard/serializat...

paulgb 4y ago

> Sawyer is a crazy class that converts a hash to an object whose methods are based on the hash's key: For those not familiar with Ruby terminology, "hash" in this context means what other languages would call a hash map, associative array, or dictionary. At first I thought it was referring to the hash of a value, and couldn't grok it.

btown 4y ago
For those on the Python side of things, https://box.readthedocs.io/en/development/ https://box.readthedocs.io/en/development/ does something similar and is a great utility to reach for when you're handling trusted JSON-like data and need an API-like interface. (The Django templating language does this too, albeit in a more magical way: https://docs.djangoproject.com/en/4.1/ref/templates/language/#variables https://docs.djangoproject.com/en/4.1/ref/templates/language...) Just... make sure you're aware of the issue that the OP's RCE brings up!
3PS 4y ago
> "hash" in this context means what other languages would call a hash map Interesting. The only other time I've seen this term used for a hash map is in the context of Redis hashes [0], which confused me to no end when I first encountered them for the same reason. Given that Redis itself is written in C, I wonder where the term originated. [0] https://redis.io/docs/data-types/hashes/ https://redis.io/docs/data-types/hashes/
- syrrim 4y ago
  Perl, probably.
- danielheath 4y ago
  This got me curious, so I went and looked what it was called in a few languages. Perl: Hash Ruby: Hash PHP: HashMap Java: HashMap Haxe: Map Golang: map Swift: Dict Python: Dict C#: Dictionary Javascript: Object Redis: Hash Postgresql: hstore, json
  - 4y ago

whimsicalism 4y ago

It is interesting how far this discloser chose to go when investigating this vulnerability. They reproduced on their local Gitlab install, then proceeded to exploit the gitlab.com install itself and break one of their own gitlab repos. I am curious what the norms are here.

bink 4y ago
It would depend on the rules for the program. GitLab's HackerOne Rules of Engagement do not explicitly prohibit testing on gitlab.com except for attacks that are "disruptive" or access the private information of other users. I'd personally put running an RCE into that category, but it's subjective. https://hackerone.com/gitlab?type=team https://hackerone.com/gitlab?type=team
- thayne 4y ago
  > I'd personally put running an RCE into that category, but it's subjective. I think that would depend on what code is being run. If it is something innocuous that just reports back that it was run, without any data, that doesn't seem disruptive or like it would access private information.
  - nigma1337 4y ago
    It does also seem like the reporter was extremely careful as to not accidentally access user data > After seeing the pings, I immediately turned off the replication by executing the redis command REPLICAOF no one\n\n. No information from your redis server has been replicated to mine as I used nc and I got only the ping messages.
    - whimsicalism 4y ago
      I mean they caused an internal server error on the Gitlab end. I agree they definitely weren't trying to access any user data other than their own.

stusmall 4y ago

I'm curious on the comment about a customer asking on the fix status. That comment was made before the thread was public. How did the customer know? Was it independently discovered or are they disclosing some known bugs to high value customers?

lbotos 4y ago
Look at the timing: https://about.gitlab.com/releases/2022/08/22/critical-security-release-gitlab-15-3-1-released/ https://about.gitlab.com/releases/2022/08/22/critical-securi... Blog post published this was patched on 8/22. Support Engineer commented on 8/25. This is on HN today because this is "30 days after H1 issue is closed make issue public" workflow.

akkartik 4y ago

Everything old is new again. https://news.ycombinator.com/item?id=5028218 https://news.ycombinator.com/item?id=5028218 https://www.sitepoint.com/anatomy-of-an-exploit-an-in-depth-look-at-the-rails-yaml-vulnerability https://www.sitepoint.com/anatomy-of-an-exploit-an-in-depth-... (https://news.ycombinator.com/item?id=5167508 https://news.ycombinator.com/item?id=5167508) https://news.ycombinator.com/item?id=5145397 https://news.ycombinator.com/item?id=5145397