What is being complained about here?

The obvious thing that stands out to me is potential timing attacks: 'and' doing short circuit eval, and '==' exiting early when the first byte is non-equal (or the lengths are non-equal).

I have doubts that a timing attack would even be exploitable here since it's a hidden service, but I just made the string comparison constant-time to be safe: https://github.com/micahflee/onionshare/issues/3

Keep in mind that the username/password are just hex-encoded 128 bits from /dev/urandom, so they're not guessable at all without some sort of leakage attack, like a timing attack. And if anyone attempts to do a timing attack the person hosting the file will see all the requests scrolling down their terminal in real-time and can always hit ctrl-c.

There's also the bit about knowing the hidden service .onion to attack in the first place, which wouldn't be trivial to discover, especially since I envision these to mostly be very short-lived.

But all that said, this is great feedback. Keep it coming and feel free to open security issues on github.

What is the semantic difference between username and password? If they're both randomly generated for a particular resource, why not combine them into one access key field?

You nailed it. In other words, literally everything is wrong with that function.

I would agree with the other comment that I suspect it would be impossible to get accurate data for a timing attack over Tor. Not only is the connection slow, but my experience has been that bandwidth and latency can be highly variable.

With enough sampling, it should be possible. Additionally, the values you are trying to infer are pretty small (8 bytes each) so that makes it easier.

Fair point. I'll admit I'm biased because I've messed around with trying time attacks on my vulnerable code in arguably ideal scenarios and haven't had much luck. So the feasibility of this is questionable to me, but I'm willing to accept that it's an issue. In any case, the above block of code should be easily fixable.

The request is going over tor-- do timing attacks work with that? When I browse with tor my connection is so slow that I don't think I could get good data for timing attacks.

Only mentioned in passing, but: Third, we have already accepted that our design is vulnerable to end-to-end timing attacks https://blog.torproject.org/blog/one-cell-enough

It doesn't matter if your connection is slow, all that matters is that latency is fairly consistent during the attack. You can use statistics to extract high-confidence data even from noisy signals, if you run the attack long enough to get lots of timing data.

> Third, we have already accepted that our design is vulnerable to end-to-end timing attacks https://blog.torproject.org/blog/one-cell-enough

They are not talking about the same kind of timing attacks. Deanonymization attacks require vastly less latency consistency.

Oh, I see! Thanks for the context.

Would a good drop in replacement here be something like hashing the two strings to the same length and using `hmac.compare_digest` ?

https://docs.python.org/2/library/hmac.html#hmac.compare_dig...

Honestly, I don't know enough about timing attacks to be sure.

Here's some pseudo code...

    passed=true
    for (i=0; i< len(auth_username); i++) {
        if (i >= len(username) || auth_username[i] != username[i]) {
            passed = false
        }
    }
    for (i=0; i< len(auth_password); i++) {
        if (i >= len(password) || auth_password[i] != password[i]) {
            passed = false
        }
    }

    return passed

This could potentially still be used to check the length of the username and password, but that would be more difficult. Hashing the username (like you mentioned) and comparing those character by character (like above) should yield the exact same timing for any inputs.

Note: There is no guarantee that a compiler won't optimize this to non-constant time.

And even if it keeps it constant time, a processor may end up running a function like this in non-constant time, due to caching and branch prediction effects.

  Hashing the username (like you mentioned) and comparing
  those character by character (like above) should yield the
  exact same timing for any inputs.

I like to use a "constant-time compare" against an HMAC of the inputs with a random key. You can initialize the key just once, it doesn't have to be a nonce, just an unknown value. The point is the attacker no longer knows or controls what value is on 'their side' of the comparison, so timing becomes irrelevant.

For the comparison itself, I usually see |= and XOR being used and then the final value being checked to be non-zero, rather than a boolean. I think the theory is the compiler is a lot less likely the exit the loop early if you're accumulating with XOR versus a single branch flipping a boolean to be false.

Here's a good write-up with some sample code: http://codahale.com/a-lesson-in-timing-attacks/. Note in that write-up they are talking about timing attacks comparing HMAC results, but that is when the attacker has control over the HMAC result not the HMAC input. If the attacker provided value is being fed into HMAC with a secret key, then you are absolutely safe from timing attacks. If the attacker has control over the HMAC result, and you are checking if that HMAC is valid, then you are back to defending against timing attacks by trying to achieve a constant-time compare.

The only thing left to leak is the HMAC will clock differently based on the block-length of the input. But block size is rather coarse, and in this case you could limit inputs to be less than one block size in all cases.

I feel like you could have gotten your point across to him without being so snarky.

Upvoted and you're right.

Pardon my lack of knowledge, but how is this single function in the server an NSA honeypot?

He said that an NSA honeypot would be more subtle, without glaring security issues like the ones in the presented example.

Is this a stab at the fact that there's no hashing? Or that the credentials are pointless when you could just generate a random url to authenticate, since the HTTP channel is encrypted anyway?

Question: Would it help mitigate timing attacks if the function would sleep for a random amount of time?

It does add more noise, which means you need more samples to detect the timing differences from the comparison. But it's generally not accepted as a 'solution' compared to making the function constant-time.