4 ms·
Yes. I made an unbelievably stupid mistake in the code that generates forms with labelled fields. It was basically functional programming taken a little too f
by pg 18y ago
Yes. I made an unbelievably stupid mistake in the code that generates forms with labelled fields. It was basically functional programming taken a little too far: I generated the same form whether the fields were editable or not, and then later if there were no editable fields I just omitted the submit button. So anyone looking at the source of one of these pages could find a fnid that would work to modify the object displayed on it. (There's still a fnid, but it no longer does anything.)
- unalone 18y agoThat would suggest, then, that the comment points and submission points are also controllable within some form, since they were tampered with also. Is that the case? (I'm not asking because I don't believe you - I'm just wondering how those numeric fields were altered.)
- suhail 18y agoAlmost sounds like SQL Injection, perhaps no escaping on fnid?
- markbao 18y agoEdit: I'm wrong, see below. If I'm not mistaken, that had nothing to do with SQL injection. The fnid basically was the authenticator that allowed a person to edit a page, regardless of who was logged in. What about other HN-powered Arc sites? Are they vulnerable as well? I won't name names, because I'm guessing they are indeed vulnerable. Edit: Yes, they are.
- pg 18y agoNot quite. A fnid is a hash key pointing to a closure on the server.
- markbao 18y agoAh, okay. Wrong guess. Everyone can downvote me now :)
- wlievens 18y agoIndeed. fnid = function id
- ralph 18y agoWith hindsight, do you still feel fnids are a good way to go? They present an obvious bottleneck in that the server has to remember many closures for a period of time as opposed to having each client remember their share. I know you drastically reduced the use of fnids some time ago. I wonder if the remainder are a vestige that's OK to live with because life's too short, or there's fundamentally a strong argument for them I'm missing.
- pg 18y agoSometimes these closures have more stuff in them than you could conveniently put in a url. For example, they might refer to another closure describing what to do next. When you try to do something that requires you to be logged in, for example, you're given the login page, with a fnid referring to a closure describing what you were in the middle of doing. That's why after logging in you're able to continue doing whatever you were doing.
- dha 18y agoAgreed. How about making the fnid a signed cache of often used environment variables(instead of a random string), which could also be looked up via a more time consuming method if needed. Depends really on your traffic profile how much data could reasonably and cost-effectively be cached, but it could be a very sizable portion saved by retrofitting the current method.
- ambition 18y agoI think I've found and fixed for hnacademic.com, thanks to the wonderful details here.
- tptacek 18y agoIt's very much like the "mass assignment" bugs that hit Rails servers when people forget to limit the writeability of Active Record attributes.
- brlewis 18y agoEscaping is needed when you pass strings to a program that parses and interprets them. That's not part of HN's architecture AFAICT. There shouldn't be any opportunity to introduce a vulnerability similar to SQL injection.
- pg 18y agoAny field of any object could be changed this way.
- IHackedHN 18y agoYou fixed the hole with impressive speed. I've got to give you that. Out of curiosity, why did you take the site down two times before applying the fix?
- pg 18y agoThe first time Trevor restarted it a few seconds before the fixed code actually arrived on the server.
- pelle 18y agoI'm glad no major damage happened. Anyway your mention of fnid prompted me to learn a bit about arc and the arc web server which looks very interesting. Might have to play with it a bit. http://arcist.com/doc/srv.html http://arcist.com/doc/srv.html
- deleted 18y ago[deleted]
- inerte 18y agoAnd are the YC application forms secure(d)?...
- pg 18y agoAgainst this bug at least, yes.
- pg 18y agoActually on closer examination the bug was subtler than that. I did check whether the incoming request was allowed to modify each field it was trying to modify. But the code that decided whether a field was modifiable was using a four variable list to destructure on a five value description of the field. This caused the variable determining whether a field was modifiable to be bound instead to the value saying whether or not it should be displayed. Which meant every field that was displayed was modifiable.
- IHackedHN 18y agoThat's actually pretty interesting to me. I didn't notice that when I was looking at the Hacker News code. Perhaps Arc shouldn't allow a list to be destructured except with a list of the same length? Allowing differing lengths seems likely to cause subtle bugs like this.
- dfranke 18y agoFor a language of my own design I'd agree, but I think that would be rather severely contrary to the spirit of Arc.
- pg 18y agoI generally tried to err on the side of flexibility, and it seemed more flexible not to require the lengths to match. I could imagine cases where you might want to use patterns both longer and shorter than the lists they were matched against. On the other hand, I'm not sure I've ever ended up taking advantage of this.