strings might not be strings

[dimbleby]

Raising mostly to double-check my understanding of the API, but I guess this is also an implicit invitation to consider whether there's some extra validation here that c-ares might consider doing.

As I understand it, the various char * values returned by c-ares to represent strings eg for hostnames etc are strings only in the C sense: they are null terminated sequences of bytes.

c-ares makes no check and no promise that these are real strings in the sense of being valid ASCII or utf8 or whatever - right?

I've been modelling it this way for some time in the rust bindings - I return CStr to callers and let them figure out what to do with it. I consider this is a small ugliness on the API: it would be nice to be able to return a regular &str.

I guess ares_dns_rr_get_str() could plausibly check that the thing that it is getting really is a string. But I'm not expecting that this is necessarily worthwhile: as I say, I am mostly just checking that I read this right.

Thanks!

[bradh352]

DNS names are guaranteed to be printable ASCII as can be seen via the escaping logic here in ares__fetch_dnsname_into_buf():

c-ares/src/lib/ares_dns_name.c

Line 502 in d1722e6

if (!ares__isprint(c)) {

However DNS strings, the parse code is in ares__buf_parse_dns_str() here:

c-ares/src/lib/ares__buf.c

Line 1006 in d1722e6

ares_status_t ares__buf_parse_dns_str(ares__buf_t *buf, size_t remaining_len,

Which calls into ares__buf_parse_dns_binstr() which has this comment "XXX: Maybe we should scan to make sure it is printable?":

c-ares/src/lib/ares__buf.c

Line 973 in d1722e6

/* XXX: Maybe we should scan to make sure it is printable? */

So, now for the _binstr() variant, I don't think that comment should be true. However for the _str() variant, we probably should actually be doing this sanity check as anything calling the _str() variant, the spec says its a NULL-terminated ASCII string.

[dimbleby]

ah so we're somewhere between where I thought we were and where I would prefer to be:

• hostname values are checked as true ascii strings
• however there are other strings that could be checked, but which are not
  • eg NAPTR flags / services / regex

do you consider the current checking to be an API commitment ie can I safely assume that c-ares intends to verify the hostnames as strings? (it would be churn to start trusting that c-ares did this, only to have to change back later)

[dimbleby]

adding a bit of archaeology, looks like the hostname validation was introduced at c9b6c60, released in 1.17.2

for my very particular use case that presents a mild dilemma, because I am currently supporting c-ares as old as 1.13 (as found on Centos 8, I believe). So I can only have the rust API guarantee a real &str if I am willing to drop that support (which I might be)

[bradh352]

I think we should fix c-ares to actually make any of the _str() functions return only printable ASCII since that's what is intended. I think I put that comment in there for myself when writing the code and simply forgot to go back and address it.

Obviously though, fixing that would only work going forward. I can cherry-pick changes to the historic branches, but that doesn't mean distros will pick up any such changes, and we are only maintaining branches back to v1.23, prior to that we never branched for releases at all. We don't actually do releases for prior branches though, just backport significant bugfix and security fixes into the github branch itself. I wish distros would be willing to pull in the c-ares releases that maintain backwards API and ABI compatibility, but I understand that it is probably hard to regression test for that when system stability is so important.

Personally from a security standpoint I really wouldn't want anyone running anything prior to 1.28 when all legacy parsers were finally removed from c-ares. That said, the most significant change for any sort of possible remote exploit was made back in 1.21 with the new DNS message parsers. Supporting back to 1.13 though, that's a pretty tall order for any project.

[bradh352]

should be addressed in 75de16c

[dimbleby]

well the main thing is whether this is or is not the right change for c-ares - if you think it is then I'm inclined to agree with you so that's all good.

totally agree that in general people running ancient code is not ideal, but from my point of view so long as the API doesn't change I consider it not my business to stand in their way.

1.13 is probably ancient enough that I can declare it dropped, but requiring use of the latest c-ares when approximately no distributions provide that is probably too much. Ah well, that's for me to worry about.

Thanks!

[dimbleby]

the uri field in a ares_uri_reply as parsed by ares_parse_uri_reply is still not validated and may contain non-string-y characters

Discovered by hacking around with the fuzzer - adding this function:

static void check_string(const char *str)
{
  if (str) {
    unsigned int i;
    unsigned long len = strlen(str);
    for (i = 0; i < len; i++) {
      assert(str[i] >= 0x20);
      assert(str[i] <= 0x7E);
    }
  }
}

and calling it liberally.

I guess there is lots more the fuzzing could do compared to what it currently does, something to consider?

[bradh352]

Heh, looks like you found one one string in a DNS response that doesn't follow the normal DNS string format and therefore uses a different parser. Should be fixed in 40fb125 ...

More fuzzers and tests would be great, obviously :)