by Serguey Shinder
Profile pictures. That was the whole feature. People wanted a face next to their name in the internal tool, it took an afternoon, and for two years it was the least interesting code in the repository.
We checked the extension. If the filename ended in .png or .jpg we accepted it, wrote it to storage, and served it back from the same domain as the application, under a path with the user's identifier in it. Every one of those decisions is the obvious one to make on the afternoon you are making it.
What ended the arrangement was not an attack. It was a penetration test booked for something else entirely, and a tester who spent about ten minutes on it. He uploaded a file called something.png that contained HTML with a script tag in it. Our check looked at the last four characters of the name and was satisfied. Storage did not care. And when a browser later fetched it, the response carried a content type our storage layer had inferred from the bytes rather than the name, and the browser ran it.
It ran on our domain, which meant it ran with our cookies, in the session of whoever was unlucky enough to open that profile. In an internal tool, the people most likely to be looking at everybody's profile are the administrators.
I remember the feeling of reading his report, which was not alarm so much as a sort of retrospective vertigo. Nobody had made a mistake. There was no careless line anywhere. There was a filename check, a storage bucket, a content type and a same-origin policy, four things owned by four different parts of the system, each behaving exactly as documented, and the vulnerability lived in the gaps between them where nobody's responsibility was written down.
The fixes were unglamorous and I would now do all of them by default. Serve user content from a separate domain, so that whatever it turns out to be, it is not us. Set the content type ourselves instead of letting anything infer it. Send a disposition header that makes the browser save rather than render. Rename the file to something we generated, and stop treating anything the user typed as a statement about what the file is.
The durable lesson was about the word validation. We had validated the upload in the sense of checking a property of its name. We had never asked the only question that mattered, which is what this thing will be permitted to do at the moment somebody opens it.
A file is not dangerous when it arrives. It becomes dangerous when something decides what it is, and by then it is usually not your code making that decision.
– Serguey Asael Shinder
Leave a Reply