Posted
I have a simple rule that I want to share:
Every time a value is inserted into a string it must be properly encoded.
It doesn’t matter if you are building HTML, SQL, YAML or a URL. It needs to be formatted properly.
Don’t do this:
fetch(`/api/v3/${action}?debug=${debug}&log=${log}&auth=${auth}`)
Do this:
fetch(`/api/v3/${encodeURIComponent(action)}?debug=${encodeURIComponent(debug)}&log=${encodeURIComponent(log)}&auth=${encodeURIComponent(auth)}`)
Or even better, avoid string-based APIs and use a library that handles the encoding for you. (You can write your own helper in a couple of lines if you prefer to keep your dependency count down.)
axios.get({
url: `/api/v3/${encodeURIComponent(action)}`,
params: {
debug,
log,
auth,
},
})
Don’t do:
spec:
serviceAccountName: {{ .Values.serviceAccountName }}
Do this:
spec:
serviceAccountName: {{ .Values.serviceAccountName | toYaml }}
Don’t do:
os.system(f"{HELPER_SCRIPT} '{name}' --set count {count}")
Do this:
subprocess.check_call([HELPER_SCRIPT, name, "--set", "count", count])
The software industry is finally beginning to understand the risk of SQL injection and web frameworks are escaping HTML by default. However it appears that this knowledge has only been applied in specific scenarios. While most developers understand the need to escape values substituted into HTML and SQL it seems that we haven’t accepted general lesson.
The important thing to realize is that every string has a format. It may be “human readable text” or it may be “HTML” but it is always something. Whenever you interpolate a value it needs to be properly serialized into the target language.
Following this simple rule will save time that would be wasted on on hard-to-track down bugs and prevent security vulnerabilities.
Every bold word is critical to the rule.
Absolutely every time. Just do it. Every time you decide not to properly encode strings you are taking a risk of introducing a bug or vulnerability. You are likely right most of the time, but you will eventually be wrong and pay the price.
It doesn’t matter if you are building a string using a concatenation operator, interpolation, a formatting library or text templates; every value needs to be properly encoded.
It may be tempting to avoid escaping values which come from trusted sources. However this is a bad idea because the source of the value may change as your product evolves. Just escape everything and you’ll save yourself a lot of work whenever you change a previously trusted value to be user-configurable. Or more likely, you will avoid bugs and vulnerabilities when someone fails to realize that the code needs to be updated.
- Even changes to trusted values can trigger bugs which take time to track down and can even cause outages.
- Values that are trusted now may be made user-controllable in the future as product requirements change.
You may know that this value is limited to a restricted range by a different system. For example you know that the API fronted requires this value to be [a-zA-Z]{3,32}. Therefore you think simple interpolation is safe. However relying on this is a bad idea.
- Value ranges change over time, someone may decide that
- is a great username character and starts allowing it. Now your code breaks, likely creating a security vulnerability.
- You may incorrectly judge the encoding requirements. For example the above may seem safe to substitute into YAML, however it is easy to forget that the keyword
false is a different type than kevin and may trigger bugs or crashes.
Rather than evaluating whether each and every string needs to be escaped it is far easier to escape everything.
- Easier for code reviewers to verify.
- Easier for automated tools to verify.
- Requires less thinking by the author.
- Avoids vulnerabilities when the author makes a mistake.
- Avoids vulnerabilities when the source data changes, violating an assumption.
I mean it. No excuses.
Proper means no shortcuts and no hacks. Putting single quotes around a substituted string isn’t proper. Use a correct serializer for the target language.
I say “encode” instead of “escape” because “escape” implies a string-to-string conversion. However this rule is more general than that. If the target language is JSON then you can encode a dictionary in the source language into a JSON dictionary. I think it also helps understanding to realize that you are serializing a value, rather than just mashing various strings together or escaping a string to be safe in another format.
The best alternative is to use a proper serialization API. These interfaces remove the need for interpolating strings all together. If you are generating a URL use a library that generates URLs and if you are generating XML use an XML generator. For example with SQL it is better to use parametrized statements then try to serialize into a SQL literal.
Another interesting approach is typed strings. This has been an idea for a long time with “taint” in Perl and has been very successful with ActiveSupport::SafeBuffer. However neither of these are perfect.
Perl’s taint assumes that data is either “good” or “bad”, but proper encoding requires more nuance. A value can be safe for JSON but unsafe for HTML. What we really want to track is a format. Maybe strings should default to “unknown” format and when interpolating the string would be suitably encoded. If unknown is of type “unknown” and you evaluate bold = html"<b>{unknown}</b>" then unknown needs to be HTML escaped because “unknown” isn’t valid HTML. If you then evaluate p = html"<p>{bold}</p>" no additional encoding is required because bold has a type of HTML. if you then run json"\{\"html\": {p}\}" the HTML needs to be serialized to a JSON string because HTML is not valid JSON.
While this idea is intriguing I’m not convinced it is the best solution. I think we are relying too much on strings. Strings are a core type of just about every programming languages but maybe that is a mistake, maybe we shouldn’t have a generic string type but different types for each format. React excels here. Instead of generating HTML primarily as string templating the programmer generates an AST and the language manages proper serialization (or in the client-side case properly translating the AST into the DOM). Not only is this naturally safe and bug-resistant but it is also more natural for developers to work with data structures rather than just bashing strings together.