Hacker News Reader

Comment by AmericanChopper | Hacker News Reader

At a glance my first though was "there's probably SQLi here", but it's not obvious to me that attacker controlled parameters could actually be used to exploit this. The ID and pagination parameters all seem to be ints, and it's not obvious to me that an attacker could turn them into arbitrary strings. Is custom_filters.phrase the attacker controlled string?

View on HN

There's nothing in that code that says that any of those parameters have to be ints. They're certainly expecting ints, but there's no casts in that code that would enforce that. Presumably somewhere upstream they're just passing directly from the POST/querystring into these API functions.

Remember: Ruby is a dynamically typed language.

Sounds like a place where static types would have helped...

Not that one should rely entirely on them for these sorts of situations.

Static types don't help with string interpolation.

They help with ensuring you're not interpolating things that can be strings where you're only expecting numbers.

You'd expect the ID parameter to be typed (at a minimum from the database). It's not clear from the code in the article whether the pagination parameters are type checked (or whether they'd raise a type error at any point if set to strings).

It's definitely bad code, but if you're going to write an article which so heavily implies "this idiot's bad commit is why a breach occurred", then whether the code is actually exploitable seems worth investigating first imo.

Again, it's Ruby. Unless you do unusual things, there's no type checking beyond duck typing.

We can't tell from just that commit whether that code is definitely exploitable, but they'd have to be doing non-default things to make it not exploitable.

so once again using sane typing systems would save the day?

It'd have helped this specific case where we can know that all the arguments must be numbers, but you're going to hit the same injection issue as soon as you need to allow a string argument.

The general fix is bound parameters, not typed arguments.

porque-no-los-dos.gif

I guess that there's a principled argument against strong typing, but I can't think of one against bound parameters. :D

Not an argument per-se, but there were cases where I had to manually build raw query parameter-values when using the `IN(v1, v2, v3)` operation. Many libraries still do not support passing in a list parameter as of 2021, even newer ones. For example, the Rust MySQL library.

I felt much safer doing that knowing the parameters I'm concatenating are integers.

You could probably use a static type system to ensure you're not allowing string arguments from untrusted input and possibly even allow string constants to be used as arguments. Not that I think this provides any sort of advantage (or that I've even seen this), but it seems like it would be possible and work.

Reply on news.ycombinator.com