Hacker News Reader

Comment by rossdavidh | Hacker News Reader

So, essentially he is saying: 1) go ahead and accept the risky thing from your users, and store it right there in your database, but 2) make sure that you remember, in every single place in your code where you read that out of the database, to treat it properly, and 3) make sure that every other programmer, now or at any time in the future, remembers to do this also, in any code they write which reads user input out of the database and puts it on the screen.

What a bad idea. Don't leave landmines there for other maintainers of the code to step on. Especially because the other maintainer may actually be you, six months or a year from now.

Sanitize your inputs. Also, escape your outputs.

View on HN

That simply does not work. You can't sanitise, escape and reproduce correctly all at the same time.

Say you run a blog. I post a comment saying "But in this case, B<A!"

This is clearly dangerous input! But it is also exactly what I wanted to say. How do you sanitise this? Change < to < in the database? Now you have to remember to NOT escape that again when outputting! And you have to make sure that, say, your text resources in your UI are all also escaped the exact same way, or you have to remember to escape them DIFFERENTLY than user-provided input.

Or maybe you "sanitise" by stripping out dangerous characters like "<". Now you have broken my comment.

The only strategy that is at all maintainable is to store the comment as received, and to escape on output. Anything else is massively fragile or broken.

Thanks for explaining this better than I could.

Maybe I'm overengineering, but couldn't you store the sanitized version as the normal value, and also store and make publicly available the original unsanitized value in an ominously and obviously named key (say, dangerouslyUnsanitizedValue) that happens to be easily greppable/lintable?

The idea that one string is more dangerous than another is the problem.

How would you determine which value to display? It seems to suffer from the same issue where if you display the sanitized value then the comment is still missing necessary characters, but if you use the unsanitized value then your application will be vulnerable to XSS.

I think you are overengineering ;)

Plain text can contain anything and it shall be treated as such, it is that simple.

As for security, don't assume everything in your database came from a trusted source. Maybe there are remains from an old version of your code that didn't sanitize, maybe you improperly used admin tools that bypassed checks.

In most cases, that would be overengineering, but it is an entirely plausible solution if you happen to have a case where you need to allow the user to enter things like angle brackets, and for some reason you cannot escape them.

> You can't sanitise, escape and reproduce correctly all at the same time.

That's why you do them at different times...

Let's go to the example:

> This is clearly dangerous input!

That's not clear at all. There is a set of values allowed for a comment, this one is probably within them, while, for example, an empty value usually isn't, as isn't and invalid UTF8 sequence. This one should pass sanitization as is.

> Change < to < in the database?

You escape it when converting into HTML. It's not the same as sanitization.

I think what you are saying is to validate, not sanitize.

Sanitize (as I have understood it) usually means to "modify to be safe", while you are talking about rejecting invalid responses.

If your rules say comments will be truncated to 1000 chars, yo do it on the input. If your rules say all prices are in dollars, but your frontend accepts other currencies, you convert on the input, and overstaff the consumer support.

Honestly, those names mean a lot of different stuff to different people. It's not good that there are so many, it's more a consequence of the widespread of bad practices.

I think what people thinking of the term like me (sanitize means modify, validate means accept/reject) will think is that if your rule is "comments will 1000 chars or less", then the validate reasoning would say reject the 1001 chars comment, while the sanitize reasoning would say truncate to 1000 chars.

Do you have a different reading of the terms?

You seem to be agreeing with me. I am saying to escape, not sanitise. What I described is sanitising.

> This is clearly dangerous input! ...

You are missing the point.

You should sanitize the input when possible, so that numbers are really numbers, strings are really strings, slugs and similar are cleaned... But of course you can't clean text so that it will be safe when displayed. After all, `<` is only problematic if you are displaying the text as HTML, which, while common, is not a given.

When displaying anything, you should however use a _framework_ that doesn't allow you to display anything that would not be safe (unless you use some function with "UNSAFE" or "DANGEROUS" in its name). For example React does that, and others too.

There are many different kinds of attacks and the less leeway an attacker has, the safer you are. So sanitize both, input and output.

The solution: don't try to reproduce input exactly. That's a weird thing to want in general anyhow - what exactly are reproducing so exactly? Text? Including Markup? How about some animation thrown in? Maybe Interactivity? Hey, let's just reproduce arbitrary executable code accurately?

The whole point of sane sanitization is that you don't need to reproduce all that stuff exactly. Pick a small domain, and reproduce that. Often, it's OK to reproduce approximately; e.g. not worrying about things like retaining multiple consecutive whitespaces, or perhaps leading/trailing whitespace, or whatever.

The point of sanitization is to make it easier not to make a dangerous mistake accidentally. If you have an input that needs to support layout, that's a pain. But if you can live with just text - so much the better. If you do need to support markup; then I don't see the wisdom in sanitizing it late; that's just asking for bugs to lead to security issues.

Frankly the whole tradeoff is nonsensical. These aren't mutually exclusive alternatives, and don't even really address the same issues. Yes, you should sanitize (and validate) your input. And you also need to escape output as appropriate.

If the point is that it's not wise to skip escaping because you "know" the input is safe due to sanitization - then sure, while theoretically sometimes sound, that's pratically a nasty bug waiting to happen. Don't do that, sure.

So are you saying I should just not ever want to say "A < B"?

No; that's just you picking an absurd example rather than being practical.

Pick a reasonable domain for each input field, considering what kind of input is useful, and what kind of usage in output (i.e. plain text output is likely much less risky than rich text). There's rarely a reason to ban < in plain text; but retaining stuff like zero-width joiners or rtl-ltr-transitions is likely less valueable, and potentially an issue with in things like usernames or email addresses (because they make it trivial to make apparently identical usernames). Similarly, if you're storing a telephone number and want to retain spaces - are you going to retain nul-chars too?

Not all input should allow arbritrary plain text. I'd guess most don't, and lot's of input is at least rich text nowadays (not to mention images and other media - you think it's a good idea to just reproduce an arbitrary image exactly?).

”Change < to < in the database?”

Of course not. The fact that “<“ is risky isn’t part of the string, it’s part of the output format (HTML).

If you were to write that string to json or csv, you would have to special-case double quotes. In. POSIX shell, asterisks and question marks need special attention, etc.

No, there is nothing to remember. Every decent HTML templating engine these days will handle all free form text as unsafe and escape it automatically. The same for SQL libraries.

Input sanizitation doesn't work, because it doesn't know what is dangerous and what is not dangerous. That depends completely on the output domain, and at the point where the inputs are received, the output domain is often unknown. Data can flow through many layers of business logic and then be passed to an SQL query, an HTML templating engine or anything else.

If you don't consider database strings to be free form text when constructing HTML, then there's a good chance there will be vulnerabilities anyway, regardless of whether any sanitization has been applied.

The article is fine.

> Input sanizitation doesn't work, because it doesn't know what is dangerous and what is not dangerous.

This is only true if you write or use shitty validation rules. You act like its impossible to do it right...it is not.

This has absolutely nothing to do with shitty validation rules. It has to do with what you're outputting it as.

If you're outputting it to HTML, commas are fine. If you're outputting it to CSV, commas are bad. And your validation rules suck if you don't allow commas in any text field because it might be output to CSV someday.

Unicode RTL override can be dangerous in filenames (in the sense of confusing humans, not computers). It is necessary to preserve in content management systems dealing with bidi content.

Of course you can try to have different validations on your model, but then you need to make sure to know all output domains on the model level instead of doing it when handing over to the view.

> Sanitize your inputs.

Sadly, there is no "the sanitization". JSON, SQL, HTML, CSS, and URI (and the future formats not invented yet) all require different escape schemes, so while you can indeed filter out anything that can be interpreted as an escape sequence in any of those formats, that's not something you can always do.

Instead, render your data properly (yes, that includes escaping whatever you're outputting).

I particularly enjoy React's dangerouslySetInnerHTML syntax

``` <div dangerouslySetInnerHTML={__html: '<p>hello world</p>'} /> ```

Like they're BEGGING you to never use it but understand sometimes it may be necessary or at least is needed to avoid worse hacks to achieve the same function.

I've taken to using prefixes like DANGEROUS or UNSAFE in various parts of our codebase to better indicate to the user where extra caution is needed.

Or treat your database as a user and don't blindly accept what it gives you without validating it.

At a previous job I implanted a user record that would have given me admin credentials on the next db migration (which happened pretty regularly) because the developer of the migration tool said "Why should I not trust this data, it came from the database". It was "sanitized" by the app for it's intended use, but not for the ETL tool.

If you sanitize your inputs you automatically create the assumption that the database is "safe", but you also have to sanitize it for every potential future usecase that the data might be used for, which is not clear when you are writing the sanitization code. Can you foresee every type of use the data will have in the future? can you know every ETL step that will be written 5, 10 years down the line? If not, it's safer to treat the data as untrusted, and if you are going to do that anyway it's a whole lot easier to just not sanitize since you will otherwise deal with double-sanitization or double-escaping.

"...what it gives you..."

Right there is the flaw. It's not "you", it's "any programmer who ever works with this system, now or any time in the future.

Programmers generally agree that user input is untrusted, right?

My point is that should just be extended to the database, and even where it isn't, it sorta already is for attacks.

"The risky thing" here is a text string. So yes, you should be able to accept an arbitrary string and store it in your database without shooting yourself in the foot, and the same when you read it out of your database and put it in HTML, or JSON, or CSV, or XML, or ASN.1, or protobufs, or in another database.

Handling escaping in HTML or REST API is web dev 101, this shouldn't be controversial.

One of my most important rules:

Whenever you say, think, or imply something starting with "So from now on we just have to remember...", you're really saying "Let's decide this part of the system will always keep breaking!".

> So, essentially he is saying: 1) go ahead and accept the risky thing from your users, and store it right there in your database

Yes. It's hard to know what "risky" is when you're taking input. You don't have the context of its use. What if someone is discussing html in a comment and wants to refer to an example? Or the same for SQL. You're going to be using a heuristic to try and "guess" this, and you're forever going to be trading off annoying your users with security, which is never a good place to be in.

> 2) make sure that you remember, in every single place in your code where you read that out of the database, to treat it properly (and 3 more of the same)

No. You simply do not use unsafe methods of mixing this data with your output. Use any remotely modern markup templating system which has a way of tracking the escaped status of text and auto-escaping it when necessary. Use an ORM or at least a database connector which has inbuilt parameter escaping support. Do not just do regular string formatting with this kind of stuff, and don't hire the sort of people who do.

> Don't leave landmines there for other maintainers of the code to step on

You don't know what is a landmine until you know the context of its use.

It is much easier to secure a perimeter at one point and know that everything one side of it is safe and everything the other side of it is not safe. The only practical place to put this perimeter is the point of use/output/whatever you want to call it. Having several different places where this data gets mangled and lacking clarity of exactly which pieces of data are safe and which are not at any one point is a recipe for disaster.

"But we'll also escape everything on output" - this leads to double-escaping and weird bespoke hacks to work around the resulting artefacts, which themselves will likely open up holes.

And this is not even touching on the idea of a data field's safeness "living with" the data. "Field xyz is safe, we sanitize it on input" - cool, but we only started sanitizing the input in September 2019, so any fields from before that are unsafe.

I like the way react does it:

dangerouslySetInnerHTML={{__html: variable}}

Not only can you easily see every place in the code that is using this construct, but, the framework provides a warning.

I definitely don't think it's bad to sanitize HTML content. but in general MOST text in a web app should be just text, and rendered with whatever HTML it contains. In very few places should the web application give user rich-text (aka HTML) access. In any place that the application does that, sanitization should be used.

Yes. And he's saying the right thing. You can't sanitize for all your possible usages.

No, nobody has to remember anything. You use prepared statements for your database, you use autoescaping templates for the output, and it just happens automatically. I wrote about this four years ago: https://palant.de/2016/03/02/why-you-should-go-with-secure-b...

On the other hand, when you "sanitize" input you get immensely complex code. Whenever you get some data in, you have to remember to sanitize it. You have to know in advance how that data is going to be used and what might be problematic there. Worse yet, as your usage of that data changes (or your understanding of the problems), the data sanitization has to change as well - both for existing data in the database and everywhere where this data comes in.

I've seen codebases doing this, where it's impossible to tell whether a particular piece of code is a vulnerability without looking up tons of context. That's the minefield for other maintainers. Don't do this.

Nope, that doesn't work, because you can never know what context your data will be used it so you can't know what's safe or not up front:

What if your output isn't always HTML? Now you need to know to decode the database AND reencode in the destination format.

Or maybe just treat strings and user input as opaque objects and leave it to the libraries to escape them.

Reply on news.ycombinator.com