Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix of null pointer p->question dereferencing #5998

Closed
wants to merge 0 commits into from

Conversation

KulykIevgen
Copy link

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.

@xnox
Copy link
Member

xnox commented May 22, 2017

@@ -2269,6 +2269,9 @@ int dns_packet_is_reply_for(DnsPacket *p, const DnsResourceKey *key) {
if (r < 0)
return r;

if (!p->question)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it's more usual to do p->question == NULL comparison

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. $ git grep 'if.*== NULL' brings up about 229 mostly in udev, and many of them are probably false positives.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -2269,6 +2269,9 @@ int dns_packet_is_reply_for(DnsPacket *p, const DnsResourceKey *key) {
if (r < 0)
return r;

if (!p->question)
return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@poettering poettering added resolve reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 22, 2017
@tyhicks
Copy link
Contributor

tyhicks commented May 23, 2017

I'm going to request a CVE for this bug from MITRE.

@evverx
Copy link
Member

evverx commented May 24, 2017

I fixed the indentation and changed the commit message a bit. #6020

@evverx
Copy link
Member

evverx commented May 24, 2017

@KulykIevgen, thanks for fixing the bug.

@evverx evverx added the bug 🐛 Programming errors, that need preferential fixing label May 24, 2017
@poettering
Copy link
Member

I'm going to request a CVE for this bug from MITRE.

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.

@msmeissn
Copy link

CVE-2017-9217 was assigned

@tyhicks
Copy link
Contributor

tyhicks commented May 24, 2017

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.

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.

This is of such a low 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.

@poettering
Copy link
Member

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...

@tyhicks
Copy link
Contributor

tyhicks commented May 24, 2017

And that way you just piss off the upstreams whose cooperation you actually should be interested in.

I greatly appreciate cooperation from upstreams and pissing you off was far from my intentions.

@keszybz keszybz added replaced-by-newer-pr and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 25, 2017
@systemd systemd locked and limited conversation to collaborators May 31, 2017
@keszybz
Copy link
Member

keszybz commented May 31, 2017

I deleted some off-topic comments.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 Programming errors, that need preferential fixing replaced-by-newer-pr resolve
Development

Successfully merging this pull request may close these issues.

None yet

7 participants