It’s a logic error rather than memory safety issue but it demonstrates other problems with the C language and the culture of C programming. A simple type system and limited flow control makes this kind of error too easy - Apple’s infamous “goto fail” suffers similarly. A practice of unit testing might have caught the error in the smtp_mailaddr implementation.
But generally, who is using system(3) or its evil cousin exec("/bin/sh", "-c", cmdline) in anything that ever comes into contact with privileges or untrusted data?
The only saving grace is that only Mbox delivery is affected; if you’re doing Maildir, you don’t have to worry about having to rebuild the whole box from scratch:
Maybe they have a specialized fuzzer for finding issues with sub process execution (see the other two vulnerabilities they’ve found at the end of the comment.), but I think its more likely that they are doing targeted reviews into this kind of bug class.
From my experience, I would start by searching for code paths that execute programs with user input and then work backwards to see if the user input is validated at all, if not then you already have your bug, otherwise you might have to do a full review on how the input is validated (which they probably did in this case and that’s how they found the logic error).
Agree. For my own code, I’d just grep for system and exec* calls and review ‘em all. If I were in the business of reviewing other code for this, I think I’d probably write a taint checker to help me look. It feels like llvm could get you really close to that, these days. It looks like there might be at least the scaffolding for that there already. Last time I had to do it as a one-off on someone else’s code, I modified the codebase I was working with to taint certain variables then highlight whether any of a group of functions acted on them.
It is now buried here with older posts. It also contains more information on how it happened, why it happened, what was done to fix it, and future plans. It’s more recent than the others and potential discussions from a detailed post-mortem seem worthwhile to me and they’re less likely to happen here now.
The reason I merged story 4gd1oz (“OpenSMTPD advisory dissected”) in to story wcgwqk (“LPE and RCE in OpenSMTPD (CVE-2020-7247)”) was that both stories covered the same topic, and were submitted within two weeks of each other. This has been how story merging has been used since it was implemented in August 2014:
Similar stories such as multiple sites reporting about the same news topic, or duplicates not found by the duplicate detection code should be able to be merged into one story.
To determine whether a two stories cover the same topic, I use the following tests:
A) Is the article for the newly submitted story a near-duplicate of the article for the previously submitted story? i.e., was the material republished or rebroadcast?
B) Does the article for the newly submitted story reference or link to the article for the previously submitted story? i.e., is it a response or follow-up? (Occasionally described as a hot take.)
C) Does the article for the newly submitted story discuss the same situation or event as the article for the previously submitted story link? i.e., are there multiple sources?
D) Does the article for the newly submitted story cover the same subject matter as the article for the previously submitted story link? e.g., is it a source code repository for a project that was the subject of a blog post?
E) In all cases, is the newly submitted story submitted no later than two weeks after the previously submitted story?
Any one of tests A-D are sufficient to merge, so long as test E holds.
Here, I merged story 4gd1oz because it passed test C: the topic (in this case the situation under discussion) for both articles being the “Qualys Security Advisory for OpenSMTPD (CVE-2020-7247)”, and test E: story wcgwqk was submitted on or about Wed, 29 Jan 2020 and story 4gd1oz was submitted on or about Fri, 31 Jan 2020. Approximately two days apart.
It is a near-certainty that later published stories will contain more information (“It also contains more information on how it happened, why it happened, what was done to fix it, and future plans”) due to the straightforward fact that more time was available to communicate with others. Variation in information content of an article doesn’t pertain to topicality, however. Stories are merged when they cover the same topic.
There has been at least one experiment to address your first reason (“It is now buried here with older posts”) due to similar report in issue 300. It was eventually reverted in commit c602b01 having been deployed less than a month.
If you’re able to describe an improvement to the hotness calculation for merged stories that you think would substantively address your last reason (“potential discussions from a detailed post-mortem seem worthwhile to me and they’re less likely to happen here now”) I’d encourage you or anyone reading to submit a PR for review. An advantage of the current hotness behavior is that a story cannot be kept alive via “trickle” of newly submitted, merged stories–a story submission behavior we do see with multiple part blog posts. Changes to how hotness is calculated on merged stories need to account for all reasons a story is merged (i.e., the merge tests articulated here). A significant majority of story merges are unremarkable and a PR should aim to incrementally make more of them so.
If you or anyone reading is able to improve the series of tests I use to decide whether a story should be merged, please voice it. Note however that where possible, I have removed discretion from the decision to merge a story via use of these tests. I’m not interested in adding discretion back in to the process. Doing so would not be an improvement. More decidability, not less.
I hope this explanation adequately describes the trade-offs involved in merging stories, why story 4gd1oz was merged, and can serve as a guide to understanding how this feature as currently implemented is and will be used.
I merge those stories I notice that other folks haven’t already suggested. If you see one you’re welcome to suggest it get merged–gerikson highlighted me for story n9ttxa here.
It’s a logic error rather than memory safety issue but it demonstrates other problems with the C language and the culture of C programming. A simple type system and limited flow control makes this kind of error too easy - Apple’s infamous “goto fail” suffers similarly. A practice of unit testing might have caught the error in the
smtp_mailaddrimplementation.But generally, who is using
system(3)or its evil cousinexec("/bin/sh", "-c", cmdline)in anything that ever comes into contact with privileges or untrusted data?Ouch.
The only saving grace is that only Mbox delivery is affected; if you’re doing Maildir, you don’t have to worry about having to rebuild the whole box from scratch:
How do people find these bugs?
I don’t think it’s just code review. Maybe fuzzing?
Maybe they have a specialized fuzzer for finding issues with sub process execution (see the other two vulnerabilities they’ve found at the end of the comment.), but I think its more likely that they are doing targeted reviews into this kind of bug class.
From my experience, I would start by searching for code paths that execute programs with user input and then work backwards to see if the user input is validated at all, if not then you already have your bug, otherwise you might have to do a full review on how the input is validated (which they probably did in this case and that’s how they found the logic error).
https://www.qualys.com/2019/12/11/cve-2019-19726/local-privilege-escalation-openbsd-dynamic-loader.txt https://www.qualys.com/2019/12/04/cve-2019-19521/authentication-vulnerabilities-openbsd.txt
Agree. For my own code, I’d just grep for system and exec* calls and review ‘em all. If I were in the business of reviewing other code for this, I think I’d probably write a taint checker to help me look. It feels like llvm could get you really close to that, these days. It looks like there might be at least the scaffolding for that there already. Last time I had to do it as a one-off on someone else’s code, I modified the codebase I was working with to taint certain variables then highlight whether any of a group of functions acted on them.
Suggest folding into https://lobste.rs/s/wcgwqk/lpe_rce_opensmtpd_cve_2020_7247
(@pushx, @alynpost)
Thank you @gerikson, I have merged the stories regarding CVE-2020-7247.
I don’t think the post-mortem should be merged into this.
Will you explain why?
It is now buried here with older posts. It also contains more information on how it happened, why it happened, what was done to fix it, and future plans. It’s more recent than the others and potential discussions from a detailed post-mortem seem worthwhile to me and they’re less likely to happen here now.
The reason I merged story 4gd1oz (“OpenSMTPD advisory dissected”) in to story wcgwqk (“LPE and RCE in OpenSMTPD (CVE-2020-7247)”) was that both stories covered the same topic, and were submitted within two weeks of each other. This has been how story merging has been used since it was implemented in August 2014:
To determine whether a two stories cover the same topic, I use the following tests:
A) Is the article for the newly submitted story a near-duplicate of the article for the previously submitted story? i.e., was the material republished or rebroadcast?
B) Does the article for the newly submitted story reference or link to the article for the previously submitted story? i.e., is it a response or follow-up? (Occasionally described as a hot take.)
C) Does the article for the newly submitted story discuss the same situation or event as the article for the previously submitted story link? i.e., are there multiple sources?
D) Does the article for the newly submitted story cover the same subject matter as the article for the previously submitted story link? e.g., is it a source code repository for a project that was the subject of a blog post?
E) In all cases, is the newly submitted story submitted no later than two weeks after the previously submitted story?
Any one of tests A-D are sufficient to merge, so long as test E holds.
Here, I merged story 4gd1oz because it passed test C: the topic (in this case the situation under discussion) for both articles being the “Qualys Security Advisory for OpenSMTPD (CVE-2020-7247)”, and test E: story wcgwqk was submitted on or about Wed, 29 Jan 2020 and story 4gd1oz was submitted on or about Fri, 31 Jan 2020. Approximately two days apart.
It is a near-certainty that later published stories will contain more information (“It also contains more information on how it happened, why it happened, what was done to fix it, and future plans”) due to the straightforward fact that more time was available to communicate with others. Variation in information content of an article doesn’t pertain to topicality, however. Stories are merged when they cover the same topic.
There has been at least one experiment to address your first reason (“It is now buried here with older posts”) due to similar report in issue 300. It was eventually reverted in commit c602b01 having been deployed less than a month.
If you’re able to describe an improvement to the hotness calculation for merged stories that you think would substantively address your last reason (“potential discussions from a detailed post-mortem seem worthwhile to me and they’re less likely to happen here now”) I’d encourage you or anyone reading to submit a PR for review. An advantage of the current hotness behavior is that a story cannot be kept alive via “trickle” of newly submitted, merged stories–a story submission behavior we do see with multiple part blog posts. Changes to how hotness is calculated on merged stories need to account for all reasons a story is merged (i.e., the merge tests articulated here). A significant majority of story merges are unremarkable and a PR should aim to incrementally make more of them so.
If you or anyone reading is able to improve the series of tests I use to decide whether a story should be merged, please voice it. Note however that where possible, I have removed discretion from the decision to merge a story via use of these tests. I’m not interested in adding discretion back in to the process. Doing so would not be an improvement. More decidability, not less.
I hope this explanation adequately describes the trade-offs involved in merging stories, why story 4gd1oz was merged, and can serve as a guide to understanding how this feature as currently implemented is and will be used.
Does that only happen by request? I see other stories about the same topic that are not merged.
I merge those stories I notice that other folks haven’t already suggested. If you see one you’re welcome to suggest it get merged–gerikson highlighted me for story n9ttxa here.
I just wanted to make sure I was understanding this all correctly. Thanks for the very detailed explanation of everything.
I fully agree with @fro here.
The CVE was “am I affected? no? ignore. yes? patch.”
This is potentially interesting for anyone who writes software :)