Bugfix of null pointer p->question dereferencing#5998
Conversation
| if (r < 0) | ||
| return r; | ||
|
|
||
| if (!p->question) |
There was a problem hiding this comment.
I thought it's more usual to do p->question == NULL comparison
There was a problem hiding this comment.
@xnox no, we actually do rely on C's "downgrade-to-bool" feature for pointers (but not for numerics), hence the code in the patch is correct and how it should be done. (also see CODING_STYLE)
There was a problem hiding this comment.
ok. $ git grep 'if.*== NULL' brings up about 229 mostly in udev, and many of them are probably false positives.
There was a problem hiding this comment.
yeah, our codebase isn't really clean in this regard... But for new code we try to follow the same style.
That said we aren't overlay strict on this really anyway. The 8ch indenting otoh matters more
| return r; | ||
|
|
||
| if (!p->question) | ||
| return 0; |
There was a problem hiding this comment.
patch looks good, but the indentation is off, we strictly use multiple of 8 space indentation! Any chance you can rework this and force push? Looks great otherwise
|
I'm going to request a CVE for this bug from MITRE. |
|
I fixed the indentation and changed the commit message a bit. #6020 |
|
@KulykIevgen, thanks for fixing the bug. |
Why? I fail to see why this was security relevant. Yes, you can make resolved abort, but it's automatically started again on the next local request, and you cannot use it to insert code or to read data you shouldn't be able to read. This is of such a low impact I fail to see where the benefit of the bureaucratic effort is... Except if your currency is CVEs, but I really hope it isn't. |
|
CVE-2017-9217 was assigned |
It is a nice feature that resolved is automatically started again on the next local request. However, the new process still has the same bug and can still be crashed with another crafted DNS response. The new process doesn't have the same state as the original process (I would assume that the cache is gone) so the crash isn't zero impact.
That's the nice thing about CVE identifiers. Distros and administrators get the ability to triage the CVE and independently consider the impact to their users. IMO, you shouldn't see the assignment of a CVE as a negative thing. The bug exists whether or not a CVE is assigned. The assignment of a CVE allows for people to consider what this issue means for them. |
Well, that makes no sense. You don't assign CVEs to every single random bugfix we do, do you? So why this one? I understand your currency is CVEs, but this just makes CVEs useless. And hardly anymore useful than a git history... I mean, I am fine with security bureaucracy if it actually helps anyone, but you just create noise where there shouldn't be any. And that way you just piss off the upstreams whose cooperation you actually should be interested in. Your at least made sure that my own interest in helping your efforts goes to zero... |
I greatly appreciate cooperation from upstreams and pissing you off was far from my intentions. |
|
I deleted some off-topic comments. |
There was a bug (dereferencing of null pointer) inside function dns_packet_is_reply_for. This function located inside resolved-dns-packet.c file. I added the check before dereferencing at line 2272 of that file. Now the bug is fixed and my local copy of systemd-resolved doesn't crash any more.