4 ms·

Why does your ntpd have a json dependency?

danudey 2y ago
This is a good question to ask, especially in the age of everything pulling in every possible dependency just to get one library function or an `isNumeric()` convenience function. The answer is that there is observability functionality which provides its results as JSON output via a UNIX socket[0]. As far as I can see, there's no other JSON functionality anywhere else in the code, so this is just to allow for easily querying (and parsing) the daemon's internal state. (I'm not convinced that JSON is the way to go here, but that's the answer to the question) [0] https://docs.ntpd-rs.pendulum-project.org/development/code-structure/#observability-task https://docs.ntpd-rs.pendulum-project.org/development/code-s...
- motrm 2y ago
  If the pieces of state are all well known at build time - and trusted in terms of their content - it may be feasible to print out JSON 'manually' as it were, instead of needing to use a JSON library, print "{" print "\"some_state\": \""; print GlobalState.Something.to_text(); print "\", "; print "\"count_of_frobs\": "; print GlobalState.FrobsCounter; print "}"; Whether it's worth doing this just to rid yourself of a dependency... who knows.
  - syncsynchalt 2y ago
    Even better to just use TSV. Hand-rolling XML or JSON is always a smell to me, even if it's visibly safe.
    - hackernudes 2y ago
      Do you mean TLV (tag-length-value)? I can't figure out what TSV is.
      - FredFS456 2y ago
        Tab Separated Values, like CSV but tabs instead of commas.
    - masklinn 2y ago
      Hand-rolling TSV is no better. The average TSV generator does not pay any mind to data cleaning, and quoting / escaping is non-standard, so what the other wide will do with it is basically playing russian roulette. Using C0 codes is likely safer at least in the sense that you will probably think to check for those and there is no reason whatsoever for them to be found in user data.
  - fiedzia 2y ago
    > If the pieces of state are all well known at build time - and trusted in terms of their content .. than use library, because you should not rely on the assumption that next developer adding one more piece to this code will magically remember to validate it with json spec.
    - maxbond 2y ago
      No magic necessary. Factor your hand-rolling into a function that returns a string (instead of printing as in the example), and write a test that parses it's return with a proper JSON library. Assert that the parsing was successful and that the extracted values are correct. Ideally you'd use a property test.
  - mananaysiempre 2y ago
    That’s somewhat better than assembling, say, HTML or SQL out of text fragments, but it’s still not fantastic. A JSON output DSL would be better still—it wouldn’t have to be particularly complicated. (Shame those usually only come paired with parsers, libxo excepted.)
  - Gigachad 2y ago
    This looks like the exact kind of thing that results in unexpected exploits.
    - Spivak 2y ago
      Hand rolled JSON input processing, yes. Hand rolled JSON output, no. You're gonna have a hard time exploiting a text file output that happens to be JSON.
      - comex 2y ago
        > You're gonna have a hard time exploiting a text file output that happens to be JSON. If you’re not escaping double quotes in strings in your hand-rolled JSON output, and some string you’re outputting happens to be something an attacker can control, then the attacker can inject arbitrary JSON. Which probably won’t compromise the program doing the outputting, but it could cause whatever reads the JSON to do something unexpected, which might be a vulnerability, depending on the design of the system. If you are escaping double quotes, then you avoid most problems, but you also need to escape control characters to ensure the JSON isn’t invalid. And also check for invalid UTF-8, if you’re using a language where strings aren’t guaranteed to be valid UTF-8. If an attacker can make the output invalid JSON, then they can cause a denial of service, which is typically not considered a severe vulnerability but is still a problem. Realistically, this is more likely to happen by accident than because of an attacker, but then it’s still an annoying bug. Oh, and if you happen to be using C and writing the JSON to a fixed-size buffer with snprintf (I’ve seen this specific pattern more than once), then the output can be silently truncated, which could also potentially allow JSON injection. Handling all that correctly doesn’t require that much code, but it’s not completely trivial either. In practice, when I see code hand-roll JSON output, it usually doesn’t even bother escaping anything. Which is usually fine, because the data being written is usually not attacker-controlled at all. For now. But code has a tendency to get adapted and reused in unexpected ways.
- deleted 2y ago
  [deleted]
orf 2y ago
Would you rather it had a JSON dependency to parse a config file, or yet another poorly thought out, ad-hoc homegrown config file format?
- itishappy 2y ago
  It uses TOML for configuration.
  - orf 2y ago
    Cool, thats why this is a hypothetical question
- akira2501 2y ago
  > yet another poorly thought out, ad-hoc homegrown config file format OpenBSD style ntpd.conf: servers 0.gentoo.pool.ntp.org servers 1.gentoo.pool.ntp.org servers 2.gentoo.pool.ntp.org servers 3.gentoo.pool.ntp.org constraints from "https://www.google.com" listen on * I mean, there's always the possibility that they used a common, well known and pretty decent config file format. In this particular case, this shouldn't be the thing that differentiates your ntpd implementation anyways.
  - IshKebab 2y ago
    That config file perfectly illustrates the point. There's no need for it to be custom, and require me to waste time learning its syntax when it could just be JSON or TOML. Honestly I would even take YAML over that and YAML is the worst.
    - xorcist 2y ago
      You still have to learn the syntax even if it is expressed in json or yaml. Perhaps stating the obvious, but not every json object is a valid ntp configuration. The configuration object will always and by definition be proprietary to ntp. Expressing it as plain text allows for a trivial parser, without any of the security implications of wrapping it in a general language language ("should this string be escaped?", "what should we do with invalid utf8?"). The more simple format has survived over thirty years, is trivial to parse by anyone, and does not bring any dependencies that needs maintaining. That should count for something.
rnijveld 2y ago
I don’t think our dependency tree is perfect, but I think our dependencies are reasonable overall. We use JSON for transferring metrics data from our NTP daemon to our prometheus metrics daemon. We’ve made this split for security reasons, why have all the attack surface of a HTTP server in your NTP daemon? That didn’t make sense to us. Which is why we added a readonly unix socket to our NTP daemon that on connecting dumps a JSON blob and then closes the connection (i.e. doing as little as possible), which is then usable by our client tool and by our prometheus metrics daemon. That data transfer uses json, but could have used any data format. We’d be happy to accept pull requests to replace this data format with something else, but given budget and time constraints, I think what we came up with is pretty reasonable.
- stavros 2y ago
  If you're only dumping a string, couldn't you replace this dependency with some string concatenation?
  - rnijveld 2y ago
    Probably, but we still need to parse that string on the client side as well. If you’re willing to do the work I’m sure we would accept a pull request for it! There’s just so many things to do in so little time unfortunately. I think reducing our dependencies is a good thing, but our dependencies for JSON parsing/writing are used so commonly in Rust and the way we use it hopefully prevents any major security issues that I don’t think this should be a high priority for us right now compared to the many things we could be doing.

This is a good question to ask, especially in the age of everything pulling in every possible dependency just to get one library function or an `isNumeric()` convenience function. The answer is that there is observability functionality which provides its results as JSON output via a UNIX socket[0]. As far as I can see, there's no other JSON functionality anywhere else in the code, so this is just to allow for easily querying (and parsing) the daemon's internal state. (I'm not convinced that JSON is the way to go here, but that's the answer to the question) [0] https://docs.ntpd-rs.pendulum-project.org/development/code-structure/#observability-task https://docs.ntpd-rs.pendulum-project.org/development/code-s...

motrm 2y ago
If the pieces of state are all well known at build time - and trusted in terms of their content - it may be feasible to print out JSON 'manually' as it were, instead of needing to use a JSON library, print "{" print "\"some_state\": \""; print GlobalState.Something.to_text(); print "\", "; print "\"count_of_frobs\": "; print GlobalState.FrobsCounter; print "}"; Whether it's worth doing this just to rid yourself of a dependency... who knows.
- syncsynchalt 2y ago
  Even better to just use TSV. Hand-rolling XML or JSON is always a smell to me, even if it's visibly safe.
  - hackernudes 2y ago
    Do you mean TLV (tag-length-value)? I can't figure out what TSV is.
    - FredFS456 2y ago
      Tab Separated Values, like CSV but tabs instead of commas.
  - masklinn 2y ago
    Hand-rolling TSV is no better. The average TSV generator does not pay any mind to data cleaning, and quoting / escaping is non-standard, so what the other wide will do with it is basically playing russian roulette. Using C0 codes is likely safer at least in the sense that you will probably think to check for those and there is no reason whatsoever for them to be found in user data.
- fiedzia 2y ago
  > If the pieces of state are all well known at build time - and trusted in terms of their content .. than use library, because you should not rely on the assumption that next developer adding one more piece to this code will magically remember to validate it with json spec.
  - maxbond 2y ago
    No magic necessary. Factor your hand-rolling into a function that returns a string (instead of printing as in the example), and write a test that parses it's return with a proper JSON library. Assert that the parsing was successful and that the extracted values are correct. Ideally you'd use a property test.
- mananaysiempre 2y ago
  That’s somewhat better than assembling, say, HTML or SQL out of text fragments, but it’s still not fantastic. A JSON output DSL would be better still—it wouldn’t have to be particularly complicated. (Shame those usually only come paired with parsers, libxo excepted.)
- Gigachad 2y ago
  This looks like the exact kind of thing that results in unexpected exploits.
  - Spivak 2y ago
    Hand rolled JSON input processing, yes. Hand rolled JSON output, no. You're gonna have a hard time exploiting a text file output that happens to be JSON.
    - comex 2y ago
      > You're gonna have a hard time exploiting a text file output that happens to be JSON. If you’re not escaping double quotes in strings in your hand-rolled JSON output, and some string you’re outputting happens to be something an attacker can control, then the attacker can inject arbitrary JSON. Which probably won’t compromise the program doing the outputting, but it could cause whatever reads the JSON to do something unexpected, which might be a vulnerability, depending on the design of the system. If you are escaping double quotes, then you avoid most problems, but you also need to escape control characters to ensure the JSON isn’t invalid. And also check for invalid UTF-8, if you’re using a language where strings aren’t guaranteed to be valid UTF-8. If an attacker can make the output invalid JSON, then they can cause a denial of service, which is typically not considered a severe vulnerability but is still a problem. Realistically, this is more likely to happen by accident than because of an attacker, but then it’s still an annoying bug. Oh, and if you happen to be using C and writing the JSON to a fixed-size buffer with snprintf (I’ve seen this specific pattern more than once), then the output can be silently truncated, which could also potentially allow JSON injection. Handling all that correctly doesn’t require that much code, but it’s not completely trivial either. In practice, when I see code hand-roll JSON output, it usually doesn’t even bother escaping anything. Which is usually fine, because the data being written is usually not attacker-controlled at all. For now. But code has a tendency to get adapted and reused in unexpected ways.
deleted 2y ago
[deleted]

orf 2y ago

Would you rather it had a JSON dependency to parse a config file, or yet another poorly thought out, ad-hoc homegrown config file format?

itishappy 2y ago
It uses TOML for configuration.
- orf 2y ago
  Cool, thats why this is a hypothetical question
akira2501 2y ago
> yet another poorly thought out, ad-hoc homegrown config file format OpenBSD style ntpd.conf: servers 0.gentoo.pool.ntp.org servers 1.gentoo.pool.ntp.org servers 2.gentoo.pool.ntp.org servers 3.gentoo.pool.ntp.org constraints from "https://www.google.com" listen on * I mean, there's always the possibility that they used a common, well known and pretty decent config file format. In this particular case, this shouldn't be the thing that differentiates your ntpd implementation anyways.
- IshKebab 2y ago
  That config file perfectly illustrates the point. There's no need for it to be custom, and require me to waste time learning its syntax when it could just be JSON or TOML. Honestly I would even take YAML over that and YAML is the worst.
  - xorcist 2y ago
    You still have to learn the syntax even if it is expressed in json or yaml. Perhaps stating the obvious, but not every json object is a valid ntp configuration. The configuration object will always and by definition be proprietary to ntp. Expressing it as plain text allows for a trivial parser, without any of the security implications of wrapping it in a general language language ("should this string be escaped?", "what should we do with invalid utf8?"). The more simple format has survived over thirty years, is trivial to parse by anyone, and does not bring any dependencies that needs maintaining. That should count for something.

rnijveld 2y ago

I don’t think our dependency tree is perfect, but I think our dependencies are reasonable overall. We use JSON for transferring metrics data from our NTP daemon to our prometheus metrics daemon. We’ve made this split for security reasons, why have all the attack surface of a HTTP server in your NTP daemon? That didn’t make sense to us. Which is why we added a readonly unix socket to our NTP daemon that on connecting dumps a JSON blob and then closes the connection (i.e. doing as little as possible), which is then usable by our client tool and by our prometheus metrics daemon. That data transfer uses json, but could have used any data format. We’d be happy to accept pull requests to replace this data format with something else, but given budget and time constraints, I think what we came up with is pretty reasonable.

stavros 2y ago
If you're only dumping a string, couldn't you replace this dependency with some string concatenation?
- rnijveld 2y ago
  Probably, but we still need to parse that string on the client side as well. If you’re willing to do the work I’m sure we would accept a pull request for it! There’s just so many things to do in so little time unfortunately. I think reducing our dependencies is a good thing, but our dependencies for JSON parsing/writing are used so commonly in Rust and the way we use it hopefully prevents any major security issues that I don’t think this should be a high priority for us right now compared to the many things we could be doing.