5 ms·

> 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' comman

> 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".

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".