10 ms·

Unexpected security footguns in Go's parsers

234 points by ingve 1y ago

tptacek 1y ago
These kinds of issues (parser differentials in particular) are why you shouldn't trust Go SAML implementations that use `encoding/xml`, which was never designed for that application to begin with; I just wrote my own for my SAML. (I mean, don't use SAML to begin with, but.)
- securesaml 1y ago
  Issue is not with go's parser, but instead about processing layer using different input than verifying layer [1] We patched the gosaml2 (and other go saml libraries), by ensuring only the authenticated bytes are processed (not the original XML document). You can see the patches here: https://github.com/russellhaering/goxmldsig/commit/e1c8a5b89d1d03089aa1a0ec546b33aaf80ee02f https://github.com/russellhaering/goxmldsig/commit/e1c8a5b89... https://github.com/russellhaering/gosaml2/commit/9957448932742ee3995e2f35f4e39431a5505c5e https://github.com/russellhaering/gosaml2/commit/99574489327... > I just wrote my own for my SAML. Curious to see your implementation for SAML and XML Signatures. [1]: https://bsky.app/profile/filippo.abyssdomain.expert/post/3lezjsf6wc2os https://bsky.app/profile/filippo.abyssdomain.expert/post/3le...
Rikudou 1y ago
And people keep on hating on PHP... Case-insensitive JSON parsing is another level, one even PHP wouldn't cross.
sudomateo 1y ago
Great informational post. I've been guilty of forgetting that pesky leading comma and changing the field name.
bravesoul2 1y ago
Oof is this for people who reuse their DTO for business logic and/or share DTOs across different APIs. It never occurred to me to ever (in any language) have a DTO with fields I wish (let alone require for security) not to unmarshall. This seems doubly strange in Go the language of "Yes absolutely DO repeat yourself!" Side rant: JS (even with Typescript) pisses me off as this is unavoidable. Splats make it worse. But still a distinct DTO and business object and don't use splats would help. (Maybe make ... a lint error)
- IceDane 1y ago
  This is not unavoidable in typescript at all. It really depends a lot on how you have structured your application, but it's basically standard practice at this point to use e.g. zod or similar to parse data at the boundaries. You may have to be careful here (remember to use zod's .strict, for example), but it's absolute not unavoidable.
  - bravesoul2 1y ago
    I should have been clear. Yes it is avoidable but only by adding more checking machinery. The bare language doesnt help. In Go etc. If a struct doesn't a field foo then there will not be a field foo at runtime. In JS there might be. Unless you bring in libraries to help prevent it. You are relying on someone remembering to use zod on every fetch
    - paulddraper 1y ago
      OpenAPI can do this too
    - Toritori12 1y ago
      huh? does Java even support marshall/unmarshall json in the 'bare' language?
- commandersaki 1y ago
  Does the origin of "DTO" come from Java? I've never seen it used anywhere else.
  - paulddraper 1y ago
    DTO comes from OOP for which Java is the poster child. But you can find it many places. https://codewithstyle.info/typescript-dto/ https://codewithstyle.info/typescript-dto/
    - zaphirplane 1y ago
DidYaWipe 1y ago
pitfalls
Joker_vD 1y ago
> Maintain consistency across boundaries. When input in processed in multiple services, ensure consistent parsing behavior by always using the same parser or implement additional validation layers, such as the strictJSONParse function shown above. Or make some "entry gate"-service not only validate/authorize requestes but also re-encode them into certainly valid shape. In the example with AuthService/ProxyService from "Attack scenario 2", make the Auth Service return not a simple "yep/nope" in response, but properly re-serialized request instead (if it's allowed in). So if e.g. AuthService takes a request with two fields "UserAction" and "AdminAction" and allows the "UserAction", it would response with a request object that has "UserAction" field in it but not "AdminAction" because the service ignored that field and so did not copy it into the response.
anitil 1y ago
This was all very interesting, but that polyglot json/yaml/xml payload was a big surprise to me! I had no idea that go's default xml parser would accept proceeding and trailing garbage. I'd always thought of json as one of the simpler formats to parse, but I suppose the real world would beg to differ. It's interesting that decisions made about seemingly-innocuous conditions like 'what if there are duplicate keys' have a long tail of consequences
- shakna 1y ago
  Tangent for breaking Python's JSON parser: This has worked for five years. The docs do not say that parsing an invalid piece will result in a RecursionError. They specify JSONDecodeError and UnicodeDecodeError. (There is a RecursionError reference to a key that is off by default - but if its off, we can still raise this...) #!/bin/sh # Python will hit it's recursion limit # If you supply just 4 less than the recursion limit # I assume this means there's a few objects on the call stack first # Probably: __main__, print, json.loads, and input. n="$(python3 -c 'import math; import sys; sys.stdout.write(str(math.floor(sys.getrecursionlimit() - 4)))')" echo "N: $n" # Obviously invalid, but unparseable without matching pair # JSON's grammar is... Not good at being partially parsed. left="$(yes [ | head -n "$n" | tr -d '\n')" # Rather than exploding with the expected decodeError # This will explode with a RecursionError # Which naturally thrashes the memory cache. echo "$left" | python3 -c 'import json; print(json.loads(input()))'
  - anitil 1y ago
    This is why we can't have nice things
  - jwilk 1y ago
    Reported as <https://github.com/python/cpython/issues/135835 https://github.com/python/cpython/issues/135835>.
    - shakna 1y ago
      I did report this five years ago. And got told that CPython don't consider it a problem. Anything is allowed to raise RecursionError without documentation, in the same way anything is allowed to raise MemoryError. I don't really agree, as "surprising" is a stench in any API. But it's their project.
neuroelectron 1y ago
Been seeing these same problems in services for decades now. It's almost like they made these protocol languages exploitable on purpose.
- CactusRocket 1y ago
  I think it's just kinda dumb parsing. E.g. JSON is an extremely simple spec. Most of those issues that the Go JSON parser has, are because of specific choices of the Go implementation, not about JSON. The fact that it allows case-insensitive key matching is just insane. Also that it parses invalid XML documents (with garbage) into valid structs without returning an error is very much a problem with the parser and not with XML.
  - cesarb 1y ago
    > The fact that it allows case-insensitive key matching is just insane. It's probably a side effect of what is IMO another bad design of that language: letter casing determining field visibility, instead of using a keyword or a sigil. If your field has to be named "User" to be public, and the corresponding entry in the JSON has all-lowercase "user" as the key (probably because the JSON was defined first, and most languages have "field names start with lowercase" as part of their naming conventions), you have to either ignore case when matching, or manually map every field. They probably wanted to be "intuitive" and not require manual mapping.
    - TheDong 1y ago
      I mean, one of the more silly things here is that even when you manually map, with the tag `json:"user"`, it still ignores the case of that while deserializing. It respects it while serializing though
    - jeroenhd 1y ago
      > If your field has to be named "User" to be public, and the corresponding entry in the JSON has all-lowercase "user" as the key then you specify the key to be "user"? Isn't that the point of the ability to remap names? Except you can't, because you don't have a choice whether or not your data is deserialised with case sensitivity enabled or not. I've written plenty of Rust code to turn camelCase into snake_case and "it's too much effort" has never been a problem. It's a minor bother that helps prevents real security issues like the ones listed in this article. Even if you want to help lazy programmers, I don't think there's a good reason to confuse "User" and "uſER" by default.
e_y_ 1y ago
As someone who isn't a Go programmer, on the face of it using strings (struct tags) for field metadata seems pretty backwards compared to Rust macros (which parses the metadata at compile time) or Java annotations (which are processed at runtime but at least don't require parsing a string to break apart options). The accidental omitempty and - are a good example of the weirdness even if they might not cause problems in practice.
- deleted 1y ago
  [deleted]
- kjksf 1y ago
  For some it's stupidity. For others it's brilliance. It's one of many examples of 80/20 design in Go: 80% of functionality with 20% of complexity and cost. Struct tags address an important scenario in an easy to use way. But they don't try to address other scenarios, like annotations do. They are not function tags. They're not variable tags. They are not general purpose annotations. They are annotations for struct fields and struct fields only. Are they are as powerful as annotations or macros? Of course not, not even close. Are they as complex to implement, understand, use? Also not. 80/20 design. 80% of functionality at 20% of cost.
  - timeon 1y ago
    Starting to get feeling that 80/20 design is not good thing. Many things seems to be driven by worse is better but, looking at things like Climate Change... was it worth it?
  - Philpax 1y ago
    Go's simplifications often introduce complexities elsewhere, however, as this article demonstrates with the complexities of correctness of a stringly-typed DSL. There's no free lunch here, and the compromises Go makes to achieve its outcomes have shown themselves to be error-prone in ways that were entirely predictable at design time.
    - valenterry 1y ago
octo888 1y ago
What is "IsAdmin" doing in the "create user" request DTO in the first place? The examples seem to indicate inappropriate re-use of data models. Would it not be better to: type CreateUserRequest struct { Username string Password string } type UserView struct { Username string IsAdmin boolean } etc? No need to just have just 1 model that maps 1:1 to your DB row. This applies to all languages
- ljm 1y ago
  Aside from security issues it presents, it’s just a really bad idea to tightly couple your data model to your API like that. Any change to your DB schema is liable to become a breaking change on your API. If you need separate types for your requests and responses so be it.
- matthew16550 1y ago
  It's a concept from Fieldings REST thesis that is important to the current meaning of REST: Transfer Objects are not the Storage Objects.
- deleted 1y ago
  [deleted]
- db48x 1y ago
  Yes!
- WhyNotHugo 1y ago
  > No need to just have just 1 model that maps 1:1 to your DB row. This applies to all languages I agree with your stance, but creating lots of DTOs is more work and we developers are lazy. It's incredibly common to see a single "User" type used everywhere. This also makes all User-related functions re-usable in all contexts (again: it's the fast and lazy approach).
- delusional 1y ago
  > No need to just have just 1 model that maps 1:1 to your DB row. This applies to all languages One reason is to avoid copying data constantly. I don't just mean this from an efficiency perspective, but also (and maybe more so) from a simplicity one. If you have a library for shoving data into a struct mechanistically, but you then take the data from that struct and shove it into an additional struct, what's the point of the library? You're writing the code move the data anyway. In my dayjob I see this tendency constantly to have a lot of different very narrow structs that somehow integrate into some library, and then a TON of supporting code to copy between those structs. Only to then do very little actually useful work with any of the data at the end. I generally think you'd be happier with fatter structs that integrated less with weird "struct-filling" libraries.
aintnolove 1y ago
I know these problems are easily avoidable... but I'm finally starting to see the appeal of protocol buffers. Just to have the assurance that, regardless of programming language, you're guaranteed a consistent ser/de experience.
- nottorp 1y ago
  But since the article isn't really about parser bugs, I don't think using a different data format will save you from most of the problems described there.
- m3047 1y ago
  > you're guaranteed a consistent ser/de experience Are there that many implementations of protobuf? How many just wrap the C lib and proto compiler? Consistency can be caused by an underlying monoculture, although that's turtles all the way down because protobuf is not YAML is not JSON, etc. Off in the weeds already, and all because I implemented a pure Python deserializer / dissector simply because there wasn't one.
- chubot 1y ago
  It would nice if it were true :-) But it’s not, for reasons that have more to do with the languages themselves, than parsing e.g. C++ numbers are different than Java numbers are different than Python numbers are different than JavaScript numbers ditto for strings
  - CactusRocket 1y ago
    I imagine that one of the points of a solid protocol buffers library would be to align the types even across programming languages. E.g. explicitly force a 64-bit integer rather than "int" relying on the platform. And to have some custom "string" type which is always UTF-8 encoded in memory rather than depending on the platform-specific encoding. (I have no idea if that is the case with protobuf, I don't have enough experience with it.)
    - chubot 1y ago
em-bee 1y ago
i am not aware of any parser that does that differently, but i would also argue that this is not the job of parsers. after parsing (or before exporting) there should be a data validation step based on whitelists. so the user can send in unknown fields all they want, the code will only accept the username and firstname strings, and ignore the other ones. same with fetching data and sending it to the user. i fetch only the fields i want and create the correct datastructures before invoking the marshaling step. there are no footguns. if you expect your parser to protect you you are using it wrong. they were not designed for that. input -> parse -> extract the fields we want, which are valid -> create a data-structure with those fields. data -> get fields i want -> create datastructures with only wanted fields -> write to output format
- deleted 1y ago
  [deleted]
- nottorp 1y ago
  > this is not the job of parsers The part of the article that I read before getting annoyed at the clickbaity title is basically "if you trust external data here's how you can blame that design decision on the parser".
- securesaml 1y ago
  This is correct. In blog post they say: > Other examples exist, but most follow the same pattern: the component that does security checks and the component that performs the actions differ in their view of the input data. This would be solved (as you described), by ensuring that the downstream layer uses only contents that are verified in the security check layer. If they are using a microservice then: Security check API -> return verified data (i.e. re-serialize the verified JSON or XML into byte form, NOT the original input) -> Processing layer i.e. userCreate API uses verified data. This is the method we used in fixing the ruby-saml example. See: https://bsky.app/profile/filippo.abyssdomain.expert/post/3lezjsf6wc2os https://bsky.app/profile/filippo.abyssdomain.expert/post/3le...
fainpul 1y ago
I didn't see this mentioned in the article: wouldn't the obvious way be to make the private field private (lowercase)? (I'm not a Go developer, just tried the language casually). type User struct { Username string `json:"username_json_key,omitempty"` Password string `json:"password"` isAdmin bool } https://go.dev/play/p/1m-6hO93Xce https://go.dev/play/p/1m-6hO93Xce
- deleted 1y ago
  [deleted]
- never_inline 1y ago
  You can annotate `json:"-"` which is equivalent of @JsonIgnore
  - zimpenfish 1y ago
    Covered in the article along with a potential snafu - adding anything else (`-,omitempty`) turns it into a key "-", not the "ignore this field" indicator.
- arp242 1y ago
  That works, but then you also can't access isAdmin from another package.
  - pdq 1y ago
    You can just make an IsAdmin() public function to expose it.
- zimpenfish 1y ago
  > wouldn't the obvious way be to make the private field private (lowercase)? That may break other things - `gorm`, for example, will ignore private fields - inconvenient if you want to serialise `User` to your DB.
  - CactusRocket 1y ago
asimops 1y ago
In the case of Attack scenario 2, I do not get why in a secure design you would ever forward the client originating data to the auth service. This is more of a broken best practise then a footgun to me. The logic should be "Parse, don't validate"[0] and after that you work on those parsed data. [0]: https://hn.algolia.com/?q=https%3A%2F%2Flexi-lambda.github.io%2Fblog%2F2019%2F11%2F05%2Fparse-don-t-validate%2F https://hn.algolia.com/?q=https%3A%2F%2Flexi-lambda.github.i...
- securesaml 1y ago
  See: https://bsky.app/profile/filippo.abyssdomain.expert/post/3lezjsf6wc2os https://bsky.app/profile/filippo.abyssdomain.expert/post/3le... that was about a signature wrapping attack in crypto, but it also applies here.
glenjamin 1y ago
It’s worth noting that if you DisallowUnknownFields it makes it much harder to handle forward/backward compatible API changes - which is a very common and usually desirable pattern
- georgelyon 1y ago
  While this is a common view, recently I’ve begun to wonder if it may be secretly an antipattern. I’ve run into a number of cases over the years where additional fields don’t break parsing, or even necessarily the main functionality of a program, but result in subtle incorrect behavior in edge cases. Things like values that are actually distinct being treated as equal because the fields that differ are ignored. More recently, I’ve seen LLMs get confused because they hallucinated tool input fields that were ignored during the invocation of a tool. I’m a little curious to try and build an API where parsing must be exact, and changes always result in a new version of the API. I don’t actually think it would be too difficult, but perhaps some extra tooling around downgrading responses and deprecating old versions may need to be built.
  - yencabulator 1y ago
    It's a convenience and a labor saver, so of course it's fundamentally at odds with security. It's all trade-offs.
- physicles 1y ago
  If you’re writing a client, I could see this being a problem. If you’re writing a server, I believe the rule is that any once valid input must stay valid forever, so you just never delete fields. The main benefit of DisallowUnknownFields is that it makes it easier for clients to know when they’ve sent something wrong or useless.
  - nine_k 1y ago
    No, once-valid input can be rejected after a period of depreciation. What actually makes sense is versioning your interfaces (and actually anything you serialize at all), with the version designator being easily accessible without parsing the entire message. (An easy way to have that is to version the endpoint URLs: /api/v1, /api/v2, etc). For some time, you support two (or more) versions. Eventually you drop the old version if it's problematic. You never have to guess, and can always reject unknown fields.
jerf 1y ago
There's a lot of good information in here, but if you think this is a time to go all language supremicist about how much better your language is and how this proves Go sucks, you might want to double-check the CVE database real quick first. A lot of these issues are more attributable to plain ol' user error and the fundamental messiness of JSON and XML than Go qua Go and I've seen many similar issues everywhere. For instance, there simply isn't a "correct" way for a parser to handle duplicate keys. Because the problem they have is different layers seeing them differently, you can have the problem anywhere duplicate keys are treated differently, and it's not like Go is the only thing to implement "last wins". It doesn't matter what you do. Last wins? Varies from the many "first wins" implementations. First wins? Varies from the many "last wins" implementations. Nondeterministically choose? Now you conflict with everyone, even yourself, sometimes. Crash or throw an exception or otherwise fail? Now you've got a potential DOS. There's no way for a parser to win here, in any langauge. The code using the parser has some decisions to make. Another example, the streaming JSON decoder "accepts" trailing garbage data because by the act of using the streaming JSON decoder you have indicated a willingness to potentially decode more JSON data. You can use this to handle newline-separated JSON, or other interesting JSON protocols where you're not committing to the string being just one JSON value and absolutely nothing else. It's not "an issue they're not planning on fixing", it's a feature with an absolutely unavoidable side effect in the context of streams. The JSON parser stops reading the stream at the end of the complete JSON object, by design, and anything else would be wrong because it would be consuming a value to "check" whether the next thing is JSON or not, when you may not even be "claiming" that the "next thing" is JSON, and whatever input it consumed to verify a claim that nobody is even making would itself be a bug. Accepting user input into sensitive variables is a common mistake I've seen multiple times in a number of langauges. The root problem there is more the tension between convenience and security than languages themselves; any language can make it so convenient to load data that developers accidentally load more data than they realize. Etc. The best lesson to take away from this is that there is more than meets the eye with JSON and XML and they're harder to use safely than its ease-of-use suggests. Although in the interests of fairness I also consider case insensitivity in the JSON field names to be a mistake; maybe it should be an option, JSON can get messy in the real world, but it's a bad default. I have other quibbles but most of them are things where there isn't a correct answer where you can unambiguously say that some choice is wrong. JSON is really quite fundamentally messier than people realize, and XML, while generally more tightly specified at the grammer level than JSON is, is generally quite messy in the protocols people build on top of it.
vlowther 1y ago
[flagged]
- cesarb 1y ago
  Even if you read the whole manual, if doing the intuitive thing results in a non-intuitive result, it's usually a bad design. The "-,omitempty" thing is a bad design because it's a non-orthogonal combination of two features. The first part means to always treat the field as if it didn't exist, and the second part means to treat the field as if it didn't exist when it's empty; but both together means to treat the field as if it did exist with a nonsensical name ("-") when it's not empty. The second part is inverting the effect of the first part, instead of adding to it.
- deleted 1y ago
  [deleted]
piinbinary 1y ago
Kudos to the author for making this very clear and easy to understand. More technical writing should be like this. On another note, it's mind-blowing that a single string can parse as XML, JSON, and YAML.

234 points by ingve 1y ago

These kinds of issues (parser differentials in particular) are why you shouldn't trust Go SAML implementations that use `encoding/xml`, which was never designed for that application to begin with; I just wrote my own for my SAML. (I mean, don't use SAML to begin with, but.)

securesaml 1y ago
Issue is not with go's parser, but instead about processing layer using different input than verifying layer [1] We patched the gosaml2 (and other go saml libraries), by ensuring only the authenticated bytes are processed (not the original XML document). You can see the patches here: https://github.com/russellhaering/goxmldsig/commit/e1c8a5b89d1d03089aa1a0ec546b33aaf80ee02f https://github.com/russellhaering/goxmldsig/commit/e1c8a5b89... https://github.com/russellhaering/gosaml2/commit/9957448932742ee3995e2f35f4e39431a5505c5e https://github.com/russellhaering/gosaml2/commit/99574489327... > I just wrote my own for my SAML. Curious to see your implementation for SAML and XML Signatures. [1]: https://bsky.app/profile/filippo.abyssdomain.expert/post/3lezjsf6wc2os https://bsky.app/profile/filippo.abyssdomain.expert/post/3le...

Rikudou 1y ago

And people keep on hating on PHP... Case-insensitive JSON parsing is another level, one even PHP wouldn't cross.

sudomateo 1y ago

Great informational post. I've been guilty of forgetting that pesky leading comma and changing the field name.

bravesoul2 1y ago

Oof is this for people who reuse their DTO for business logic and/or share DTOs across different APIs. It never occurred to me to ever (in any language) have a DTO with fields I wish (let alone require for security) not to unmarshall. This seems doubly strange in Go the language of "Yes absolutely DO repeat yourself!" Side rant: JS (even with Typescript) pisses me off as this is unavoidable. Splats make it worse. But still a distinct DTO and business object and don't use splats would help. (Maybe make ... a lint error)

IceDane 1y ago
This is not unavoidable in typescript at all. It really depends a lot on how you have structured your application, but it's basically standard practice at this point to use e.g. zod or similar to parse data at the boundaries. You may have to be careful here (remember to use zod's .strict, for example), but it's absolute not unavoidable.
- bravesoul2 1y ago
  I should have been clear. Yes it is avoidable but only by adding more checking machinery. The bare language doesnt help. In Go etc. If a struct doesn't a field foo then there will not be a field foo at runtime. In JS there might be. Unless you bring in libraries to help prevent it. You are relying on someone remembering to use zod on every fetch
  - paulddraper 1y ago
    OpenAPI can do this too
  - Toritori12 1y ago
    huh? does Java even support marshall/unmarshall json in the 'bare' language?
commandersaki 1y ago
Does the origin of "DTO" come from Java? I've never seen it used anywhere else.
- paulddraper 1y ago
  DTO comes from OOP for which Java is the poster child. But you can find it many places. https://codewithstyle.info/typescript-dto/ https://codewithstyle.info/typescript-dto/
  - zaphirplane 1y ago

DidYaWipe 1y ago

pitfalls

Joker_vD 1y ago

> Maintain consistency across boundaries. When input in processed in multiple services, ensure consistent parsing behavior by always using the same parser or implement additional validation layers, such as the strictJSONParse function shown above. Or make some "entry gate"-service not only validate/authorize requestes but also re-encode them into certainly valid shape. In the example with AuthService/ProxyService from "Attack scenario 2", make the Auth Service return not a simple "yep/nope" in response, but properly re-serialized request instead (if it's allowed in). So if e.g. AuthService takes a request with two fields "UserAction" and "AdminAction" and allows the "UserAction", it would response with a request object that has "UserAction" field in it but not "AdminAction" because the service ignored that field and so did not copy it into the response.

anitil 1y ago

This was all very interesting, but that polyglot json/yaml/xml payload was a big surprise to me! I had no idea that go's default xml parser would accept proceeding and trailing garbage. I'd always thought of json as one of the simpler formats to parse, but I suppose the real world would beg to differ. It's interesting that decisions made about seemingly-innocuous conditions like 'what if there are duplicate keys' have a long tail of consequences

shakna 1y ago
Tangent for breaking Python's JSON parser: This has worked for five years. The docs do not say that parsing an invalid piece will result in a RecursionError. They specify JSONDecodeError and UnicodeDecodeError. (There is a RecursionError reference to a key that is off by default - but if its off, we can still raise this...) #!/bin/sh # Python will hit it's recursion limit # If you supply just 4 less than the recursion limit # I assume this means there's a few objects on the call stack first # Probably: __main__, print, json.loads, and input. n="$(python3 -c 'import math; import sys; sys.stdout.write(str(math.floor(sys.getrecursionlimit() - 4)))')" echo "N: $n" # Obviously invalid, but unparseable without matching pair # JSON's grammar is... Not good at being partially parsed. left="$(yes [ | head -n "$n" | tr -d '\n')" # Rather than exploding with the expected decodeError # This will explode with a RecursionError # Which naturally thrashes the memory cache. echo "$left" | python3 -c 'import json; print(json.loads(input()))'
- anitil 1y ago
  This is why we can't have nice things
- jwilk 1y ago
  Reported as <https://github.com/python/cpython/issues/135835 https://github.com/python/cpython/issues/135835>.
  - shakna 1y ago
    I did report this five years ago. And got told that CPython don't consider it a problem. Anything is allowed to raise RecursionError without documentation, in the same way anything is allowed to raise MemoryError. I don't really agree, as "surprising" is a stench in any API. But it's their project.

neuroelectron 1y ago

Been seeing these same problems in services for decades now. It's almost like they made these protocol languages exploitable on purpose.

CactusRocket 1y ago
I think it's just kinda dumb parsing. E.g. JSON is an extremely simple spec. Most of those issues that the Go JSON parser has, are because of specific choices of the Go implementation, not about JSON. The fact that it allows case-insensitive key matching is just insane. Also that it parses invalid XML documents (with garbage) into valid structs without returning an error is very much a problem with the parser and not with XML.
- cesarb 1y ago
  > The fact that it allows case-insensitive key matching is just insane. It's probably a side effect of what is IMO another bad design of that language: letter casing determining field visibility, instead of using a keyword or a sigil. If your field has to be named "User" to be public, and the corresponding entry in the JSON has all-lowercase "user" as the key (probably because the JSON was defined first, and most languages have "field names start with lowercase" as part of their naming conventions), you have to either ignore case when matching, or manually map every field. They probably wanted to be "intuitive" and not require manual mapping.
  - TheDong 1y ago
    I mean, one of the more silly things here is that even when you manually map, with the tag `json:"user"`, it still ignores the case of that while deserializing. It respects it while serializing though
  - jeroenhd 1y ago
    > If your field has to be named "User" to be public, and the corresponding entry in the JSON has all-lowercase "user" as the key then you specify the key to be "user"? Isn't that the point of the ability to remap names? Except you can't, because you don't have a choice whether or not your data is deserialised with case sensitivity enabled or not. I've written plenty of Rust code to turn camelCase into snake_case and "it's too much effort" has never been a problem. It's a minor bother that helps prevents real security issues like the ones listed in this article. Even if you want to help lazy programmers, I don't think there's a good reason to confuse "User" and "uſER" by default.

e_y_ 1y ago

As someone who isn't a Go programmer, on the face of it using strings (struct tags) for field metadata seems pretty backwards compared to Rust macros (which parses the metadata at compile time) or Java annotations (which are processed at runtime but at least don't require parsing a string to break apart options). The accidental omitempty and - are a good example of the weirdness even if they might not cause problems in practice.

deleted 1y ago
[deleted]
kjksf 1y ago
For some it's stupidity. For others it's brilliance. It's one of many examples of 80/20 design in Go: 80% of functionality with 20% of complexity and cost. Struct tags address an important scenario in an easy to use way. But they don't try to address other scenarios, like annotations do. They are not function tags. They're not variable tags. They are not general purpose annotations. They are annotations for struct fields and struct fields only. Are they are as powerful as annotations or macros? Of course not, not even close. Are they as complex to implement, understand, use? Also not. 80/20 design. 80% of functionality at 20% of cost.
- timeon 1y ago
  Starting to get feeling that 80/20 design is not good thing. Many things seems to be driven by worse is better but, looking at things like Climate Change... was it worth it?
- Philpax 1y ago
  Go's simplifications often introduce complexities elsewhere, however, as this article demonstrates with the complexities of correctness of a stringly-typed DSL. There's no free lunch here, and the compromises Go makes to achieve its outcomes have shown themselves to be error-prone in ways that were entirely predictable at design time.
  - valenterry 1y ago

octo888 1y ago

What is "IsAdmin" doing in the "create user" request DTO in the first place? The examples seem to indicate inappropriate re-use of data models. Would it not be better to: type CreateUserRequest struct { Username string Password string } type UserView struct { Username string IsAdmin boolean } etc? No need to just have just 1 model that maps 1:1 to your DB row. This applies to all languages

ljm 1y ago
Aside from security issues it presents, it’s just a really bad idea to tightly couple your data model to your API like that. Any change to your DB schema is liable to become a breaking change on your API. If you need separate types for your requests and responses so be it.
matthew16550 1y ago
It's a concept from Fieldings REST thesis that is important to the current meaning of REST: Transfer Objects are not the Storage Objects.
deleted 1y ago
[deleted]
db48x 1y ago
Yes!
WhyNotHugo 1y ago
> No need to just have just 1 model that maps 1:1 to your DB row. This applies to all languages I agree with your stance, but creating lots of DTOs is more work and we developers are lazy. It's incredibly common to see a single "User" type used everywhere. This also makes all User-related functions re-usable in all contexts (again: it's the fast and lazy approach).
delusional 1y ago
> No need to just have just 1 model that maps 1:1 to your DB row. This applies to all languages One reason is to avoid copying data constantly. I don't just mean this from an efficiency perspective, but also (and maybe more so) from a simplicity one. If you have a library for shoving data into a struct mechanistically, but you then take the data from that struct and shove it into an additional struct, what's the point of the library? You're writing the code move the data anyway. In my dayjob I see this tendency constantly to have a lot of different very narrow structs that somehow integrate into some library, and then a TON of supporting code to copy between those structs. Only to then do very little actually useful work with any of the data at the end. I generally think you'd be happier with fatter structs that integrated less with weird "struct-filling" libraries.

aintnolove 1y ago

I know these problems are easily avoidable... but I'm finally starting to see the appeal of protocol buffers. Just to have the assurance that, regardless of programming language, you're guaranteed a consistent ser/de experience.

nottorp 1y ago
But since the article isn't really about parser bugs, I don't think using a different data format will save you from most of the problems described there.
m3047 1y ago
> you're guaranteed a consistent ser/de experience Are there that many implementations of protobuf? How many just wrap the C lib and proto compiler? Consistency can be caused by an underlying monoculture, although that's turtles all the way down because protobuf is not YAML is not JSON, etc. Off in the weeds already, and all because I implemented a pure Python deserializer / dissector simply because there wasn't one.
chubot 1y ago
It would nice if it were true :-) But it’s not, for reasons that have more to do with the languages themselves, than parsing e.g. C++ numbers are different than Java numbers are different than Python numbers are different than JavaScript numbers ditto for strings
- CactusRocket 1y ago
  I imagine that one of the points of a solid protocol buffers library would be to align the types even across programming languages. E.g. explicitly force a 64-bit integer rather than "int" relying on the platform. And to have some custom "string" type which is always UTF-8 encoded in memory rather than depending on the platform-specific encoding. (I have no idea if that is the case with protobuf, I don't have enough experience with it.)
  - chubot 1y ago

em-bee 1y ago

i am not aware of any parser that does that differently, but i would also argue that this is not the job of parsers. after parsing (or before exporting) there should be a data validation step based on whitelists. so the user can send in unknown fields all they want, the code will only accept the username and firstname strings, and ignore the other ones. same with fetching data and sending it to the user. i fetch only the fields i want and create the correct datastructures before invoking the marshaling step. there are no footguns. if you expect your parser to protect you you are using it wrong. they were not designed for that. input -> parse -> extract the fields we want, which are valid -> create a data-structure with those fields. data -> get fields i want -> create datastructures with only wanted fields -> write to output format

deleted 1y ago
[deleted]
nottorp 1y ago
> this is not the job of parsers The part of the article that I read before getting annoyed at the clickbaity title is basically "if you trust external data here's how you can blame that design decision on the parser".
securesaml 1y ago
This is correct. In blog post they say: > Other examples exist, but most follow the same pattern: the component that does security checks and the component that performs the actions differ in their view of the input data. This would be solved (as you described), by ensuring that the downstream layer uses only contents that are verified in the security check layer. If they are using a microservice then: Security check API -> return verified data (i.e. re-serialize the verified JSON or XML into byte form, NOT the original input) -> Processing layer i.e. userCreate API uses verified data. This is the method we used in fixing the ruby-saml example. See: https://bsky.app/profile/filippo.abyssdomain.expert/post/3lezjsf6wc2os https://bsky.app/profile/filippo.abyssdomain.expert/post/3le...

fainpul 1y ago

I didn't see this mentioned in the article: wouldn't the obvious way be to make the private field private (lowercase)? (I'm not a Go developer, just tried the language casually). type User struct { Username string `json:"username_json_key,omitempty"` Password string `json:"password"` isAdmin bool } https://go.dev/play/p/1m-6hO93Xce https://go.dev/play/p/1m-6hO93Xce

deleted 1y ago
[deleted]
never_inline 1y ago
You can annotate `json:"-"` which is equivalent of @JsonIgnore
- zimpenfish 1y ago
  Covered in the article along with a potential snafu - adding anything else (`-,omitempty`) turns it into a key "-", not the "ignore this field" indicator.
arp242 1y ago
That works, but then you also can't access isAdmin from another package.
- pdq 1y ago
  You can just make an IsAdmin() public function to expose it.
zimpenfish 1y ago
> wouldn't the obvious way be to make the private field private (lowercase)? That may break other things - `gorm`, for example, will ignore private fields - inconvenient if you want to serialise `User` to your DB.
- CactusRocket 1y ago

asimops 1y ago

In the case of Attack scenario 2, I do not get why in a secure design you would ever forward the client originating data to the auth service. This is more of a broken best practise then a footgun to me. The logic should be "Parse, don't validate"[0] and after that you work on those parsed data. [0]: https://hn.algolia.com/?q=https%3A%2F%2Flexi-lambda.github.io%2Fblog%2F2019%2F11%2F05%2Fparse-don-t-validate%2F https://hn.algolia.com/?q=https%3A%2F%2Flexi-lambda.github.i...

securesaml 1y ago
See: https://bsky.app/profile/filippo.abyssdomain.expert/post/3lezjsf6wc2os https://bsky.app/profile/filippo.abyssdomain.expert/post/3le... that was about a signature wrapping attack in crypto, but it also applies here.

glenjamin 1y ago

It’s worth noting that if you DisallowUnknownFields it makes it much harder to handle forward/backward compatible API changes - which is a very common and usually desirable pattern

georgelyon 1y ago
While this is a common view, recently I’ve begun to wonder if it may be secretly an antipattern. I’ve run into a number of cases over the years where additional fields don’t break parsing, or even necessarily the main functionality of a program, but result in subtle incorrect behavior in edge cases. Things like values that are actually distinct being treated as equal because the fields that differ are ignored. More recently, I’ve seen LLMs get confused because they hallucinated tool input fields that were ignored during the invocation of a tool. I’m a little curious to try and build an API where parsing must be exact, and changes always result in a new version of the API. I don’t actually think it would be too difficult, but perhaps some extra tooling around downgrading responses and deprecating old versions may need to be built.
- yencabulator 1y ago
  It's a convenience and a labor saver, so of course it's fundamentally at odds with security. It's all trade-offs.
physicles 1y ago
If you’re writing a client, I could see this being a problem. If you’re writing a server, I believe the rule is that any once valid input must stay valid forever, so you just never delete fields. The main benefit of DisallowUnknownFields is that it makes it easier for clients to know when they’ve sent something wrong or useless.
- nine_k 1y ago
  No, once-valid input can be rejected after a period of depreciation. What actually makes sense is versioning your interfaces (and actually anything you serialize at all), with the version designator being easily accessible without parsing the entire message. (An easy way to have that is to version the endpoint URLs: /api/v1, /api/v2, etc). For some time, you support two (or more) versions. Eventually you drop the old version if it's problematic. You never have to guess, and can always reject unknown fields.

jerf 1y ago

There's a lot of good information in here, but if you think this is a time to go all language supremicist about how much better your language is and how this proves Go sucks, you might want to double-check the CVE database real quick first. A lot of these issues are more attributable to plain ol' user error and the fundamental messiness of JSON and XML than Go qua Go and I've seen many similar issues everywhere. For instance, there simply isn't a "correct" way for a parser to handle duplicate keys. Because the problem they have is different layers seeing them differently, you can have the problem anywhere duplicate keys are treated differently, and it's not like Go is the only thing to implement "last wins". It doesn't matter what you do. Last wins? Varies from the many "first wins" implementations. First wins? Varies from the many "last wins" implementations. Nondeterministically choose? Now you conflict with everyone, even yourself, sometimes. Crash or throw an exception or otherwise fail? Now you've got a potential DOS. There's no way for a parser to win here, in any langauge. The code using the parser has some decisions to make. Another example, the streaming JSON decoder "accepts" trailing garbage data because by the act of using the streaming JSON decoder you have indicated a willingness to potentially decode more JSON data. You can use this to handle newline-separated JSON, or other interesting JSON protocols where you're not committing to the string being just one JSON value and absolutely nothing else. It's not "an issue they're not planning on fixing", it's a feature with an absolutely unavoidable side effect in the context of streams. The JSON parser stops reading the stream at the end of the complete JSON object, by design, and anything else would be wrong because it would be consuming a value to "check" whether the next thing is JSON or not, when you may not even be "claiming" that the "next thing" is JSON, and whatever input it consumed to verify a claim that nobody is even making would itself be a bug. Accepting user input into sensitive variables is a common mistake I've seen multiple times in a number of langauges. The root problem there is more the tension between convenience and security than languages themselves; any language can make it so convenient to load data that developers accidentally load more data than they realize. Etc. The best lesson to take away from this is that there is more than meets the eye with JSON and XML and they're harder to use safely than its ease-of-use suggests. Although in the interests of fairness I also consider case insensitivity in the JSON field names to be a mistake; maybe it should be an option, JSON can get messy in the real world, but it's a bad default. I have other quibbles but most of them are things where there isn't a correct answer where you can unambiguously say that some choice is wrong. JSON is really quite fundamentally messier than people realize, and XML, while generally more tightly specified at the grammer level than JSON is, is generally quite messy in the protocols people build on top of it.

vlowther 1y ago

[flagged]

cesarb 1y ago
Even if you read the whole manual, if doing the intuitive thing results in a non-intuitive result, it's usually a bad design. The "-,omitempty" thing is a bad design because it's a non-orthogonal combination of two features. The first part means to always treat the field as if it didn't exist, and the second part means to treat the field as if it didn't exist when it's empty; but both together means to treat the field as if it did exist with a nonsensical name ("-") when it's not empty. The second part is inverting the effect of the first part, instead of adding to it.
deleted 1y ago
[deleted]

piinbinary 1y ago

Kudos to the author for making this very clear and easy to understand. More technical writing should be like this. On another note, it's mind-blowing that a single string can parse as XML, JSON, and YAML.