13 ms·

This is bad code and it's a little surprising to me that a former Facebook engineer wrote it (and subsequently became the CTO). It's been a while since I wrote

This is bad code and it's a little surprising to me that a former Facebook engineer wrote it (and subsequently became the CTO). It's been a while since I wrote any Rails but the offenses that jump out just from a cursory inspection: - large raw SQL query which could almost certainly be accomplished in a more idiomatic way with AREL or ActiveRecord - no user input sanitizing - using a regular string literal for a large text block instead of a Ruby here document - leaving a mess of commented-out code at the bottom of the method Apparently Gab isn't exactly hiring the best and brightest.

AmericanChopper 6y ago
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?
- kemayo 6y ago
  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.
  - mannerheim 6y ago
    Sounds like a place where static types would have helped... Not that one should rely entirely on them for these sorts of situations.
    - 1_player 6y ago
      Static types don't help with string interpolation.
      - mannerheim 6y ago
        They help with ensuring you're not interpolating things that can be strings where you're only expecting numbers.
  - AmericanChopper 6y ago
    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.
    - kemayo 6y ago
      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.
  - tester34 6y ago
    so once again using sane typing systems would save the day?
    - kemayo 6y ago
      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.
      - luuio 6y ago
        porque-no-los-dos.gif
        kemayo 6y ago
        I guess that there's a principled argument against strong typing, but I can't think of one against bound parameters. :D
        luuio 6y ago
        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.
      - mannerheim 6y ago
        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.
shadowgovt 6y ago
It's always worth remembering that there's a difference between employment and a university degree. Universities don't grant degrees until someone has proved they have whatever competency the degree indicates. Employment means you got paid by the company, but what you did to get paid could be lots of things... And it's pretty easy to lie on a résumé. Universities have a reputation to lose if they grant a degree to someone who goes on to mess up badly; corporations have no such ties to a former employee, and Mr. Marotto making a major error when he's no longer employed by Facebook doesn't reflect poorly on Facebook. "Former Facebook engineer" means Facebook paid him... No more and no less.
- bashwizard 6y ago
  Not to be that guy but some of the worst security vulnerabilities I've seen has been from senior devs with college degrees. Having a degree =/= you know your shit or being "immune" to absurdly stupid mistakes like this. You'd be surprised how often I've found leaked API tokens, passwords in plaintext etc on github for instance.
- mrunkel 6y ago
  > Universities don't grant degrees until someone has proved they have whatever competency the degree indicates. Universities grant degrees based on the successful completion of course requirements. I don't know if it's fair to conflate that with what you said. As long as you can get C or better in every course required, you have a degree. Does that necessarily mean that you are competent? It's an even further stretch if you think that at University cares all that much if one of their Computer Science graduates makes a mistake.
  - shadowgovt 6y ago
    > As long as you can get C or better in every course required, you have a degree. Does that necessarily mean that you are competent? It means that the university granted you a degree (and, indeed, the med student who got a D average in every class is still called "Doctor" when they finish the process). The point is: universities have a vested interest in the question "Does that degree indicate you have competence?" and incentives to change if the answer, on average, trends toward "no" for their graduates. Corporations have no particular vested interest in the question "Does the corporation's name on an ex-employee's résumé mean that employee has competence?" That's some competitor's problem, not theirs.
smoldesu 6y ago
I'm not terribly surprised on their hiring practices. I got a Gab account a few years ago for kicks, and I was banned within a week. I never got any justification for my ban, and after scouring my locked account for any offending posts I can only conclude that their moderators are horribly corrupt. I love decentralized software, but don't let Gab fool you. They're just out to grab power right now.
np_tedious 6y ago
Idk when this guy worked there, but today a Facebook product dev likely doesn't get a chance to make a mistake like this. Which is good. But it also means people who've only worked there can have some really surprising gaps
robinduckett 6y ago
Why is it surprising to you that Facebook could hire someone inept?
- abvdasker 6y ago
  I mean I did say it's only a little surprising.
ufmace 6y ago
Have you ever worked on any really large platforms or databases? IME it's pretty common that the idiomatic Arel/ActiveRecord code you might normally write can become hopelessly slow when the query gets complex and the datasets get large. I'm not trying to pick on ActiveRecord here, pretty much all ORMs I've ever used will do this sometimes. The only way to get the speed up to something acceptable may be for someone who knows SQL and the DB Schema well to write a custom SQL query that makes the most of the database indexes and optimizations. When a large product in Production is having performance issues, it's very possible that you'll write something like that in a time crunch to get the site back up. The code might not be the cleanest possible because you aren't sure how well it will work yet. It might be checked into Git because any remotely sane deployment procedure requires it. And maybe somebody years later will pore through the Git logs and come back and drop snark on you without knowing anything about what was involved in writing and releasing that code and they've never had to deal with something like that.

AmericanChopper 6y ago

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?

kemayo 6y ago
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.
- mannerheim 6y ago
  Sounds like a place where static types would have helped... Not that one should rely entirely on them for these sorts of situations.
  - 1_player 6y ago
    Static types don't help with string interpolation.
    - mannerheim 6y ago
      They help with ensuring you're not interpolating things that can be strings where you're only expecting numbers.
- AmericanChopper 6y ago
  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.
  - kemayo 6y ago
    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.
- tester34 6y ago
  so once again using sane typing systems would save the day?
  - kemayo 6y ago
    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.
    - luuio 6y ago
      porque-no-los-dos.gif
      - kemayo 6y ago
        I guess that there's a principled argument against strong typing, but I can't think of one against bound parameters. :D
        luuio 6y ago
        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.
    - mannerheim 6y ago
      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.

shadowgovt 6y ago

It's always worth remembering that there's a difference between employment and a university degree. Universities don't grant degrees until someone has proved they have whatever competency the degree indicates. Employment means you got paid by the company, but what you did to get paid could be lots of things... And it's pretty easy to lie on a résumé. Universities have a reputation to lose if they grant a degree to someone who goes on to mess up badly; corporations have no such ties to a former employee, and Mr. Marotto making a major error when he's no longer employed by Facebook doesn't reflect poorly on Facebook. "Former Facebook engineer" means Facebook paid him... No more and no less.

bashwizard 6y ago
Not to be that guy but some of the worst security vulnerabilities I've seen has been from senior devs with college degrees. Having a degree =/= you know your shit or being "immune" to absurdly stupid mistakes like this. You'd be surprised how often I've found leaked API tokens, passwords in plaintext etc on github for instance.
mrunkel 6y ago
> Universities don't grant degrees until someone has proved they have whatever competency the degree indicates. Universities grant degrees based on the successful completion of course requirements. I don't know if it's fair to conflate that with what you said. As long as you can get C or better in every course required, you have a degree. Does that necessarily mean that you are competent? It's an even further stretch if you think that at University cares all that much if one of their Computer Science graduates makes a mistake.
- shadowgovt 6y ago
  > As long as you can get C or better in every course required, you have a degree. Does that necessarily mean that you are competent? It means that the university granted you a degree (and, indeed, the med student who got a D average in every class is still called "Doctor" when they finish the process). The point is: universities have a vested interest in the question "Does that degree indicate you have competence?" and incentives to change if the answer, on average, trends toward "no" for their graduates. Corporations have no particular vested interest in the question "Does the corporation's name on an ex-employee's résumé mean that employee has competence?" That's some competitor's problem, not theirs.

smoldesu 6y ago

I'm not terribly surprised on their hiring practices. I got a Gab account a few years ago for kicks, and I was banned within a week. I never got any justification for my ban, and after scouring my locked account for any offending posts I can only conclude that their moderators are horribly corrupt. I love decentralized software, but don't let Gab fool you. They're just out to grab power right now.

np_tedious 6y ago

Idk when this guy worked there, but today a Facebook product dev likely doesn't get a chance to make a mistake like this. Which is good. But it also means people who've only worked there can have some really surprising gaps

robinduckett 6y ago

Why is it surprising to you that Facebook could hire someone inept?

abvdasker 6y ago
I mean I did say it's only a little surprising.

ufmace 6y ago

Have you ever worked on any really large platforms or databases? IME it's pretty common that the idiomatic Arel/ActiveRecord code you might normally write can become hopelessly slow when the query gets complex and the datasets get large. I'm not trying to pick on ActiveRecord here, pretty much all ORMs I've ever used will do this sometimes. The only way to get the speed up to something acceptable may be for someone who knows SQL and the DB Schema well to write a custom SQL query that makes the most of the database indexes and optimizations. When a large product in Production is having performance issues, it's very possible that you'll write something like that in a time crunch to get the site back up. The code might not be the cleanest possible because you aren't sure how well it will work yet. It might be checked into Git because any remotely sane deployment procedure requires it. And maybe somebody years later will pore through the Git logs and come back and drop snark on you without knowing anything about what was involved in writing and releasing that code and they've never had to deal with something like that.