dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.23k stars 1.35k forks source link

mansign2 applies SHA-1 timestamps if TSS returns a non-HTTP-OK response #7164

Closed drauch closed 2 years ago

drauch commented 2 years ago

If SignedCmiManifest2 is passed useSha256=true then mansign2 tries to apply a SHA-2 timestamp to the signature in TimestampSignedLicenseDom. However, if ObtainRFC3161Timestamp fails, it falls back to a non-RFC3161 timestamp and silently applies a SHA-1 timestamp without telling the outside world that it has done so.

This is a problem for us, as our TSS sometimes returns a non-200 return code due to rate limits (throttling). Instead of retrying or failing the signing operation, mansign2 silently applies a SHA-1 timestamp.

This is not following the principle of least surprise and should be mitigated. In an optimal world mansign2 could detect whether the TSS does not support RFC3161 or whether it returned a non-HTTP-OK response for a different reason. If that's not possible, it should at least inform the outside world that it has fallen back to SHA-1.

Best regards, D.R.

Forgind commented 2 years ago

Adding a message when it falls back to SHA-1 is minimal but definitely doable. Trying at least a few times should also be doable. I'm curious if we can disable the fallback entirely and just throw an exception—I don't think SHA-1 is currently in support (as of earlier this year), but it may still be used in a variety of applications. Maybe we can do that under a changewave?

drauch commented 2 years ago

The exception scenario would be best in our case, this would allow us to retry the whole operation.

Forgind commented 2 years ago

@sujitnayak, how would you prefer to solve this?

sujitnayak commented 2 years ago

Just to be clear, is this issue referring to the fallback code in the exception handler of TimestampSignedLicenseDom?

We call Win32.CertTimestampAuthenticodeLicense API in the fallback code when RFC3161 timestamping attempt fails in an attempt a generate a non-RFC3161 timestamp. I am not sure what algorithm it uses to calculate the timestamp but is the claim that this API is using SHA-1 to calculate the hash?

Thanks.

Forgind commented 2 years ago

Yes and yes, and that part seems to be true. It seems a little wrong to silently use a different timestamp to that requested.

sujitnayak commented 2 years ago

Manifest signing done via msbuild task and the Mage toolset for ClickOnce has to have identical behavior.

I will reach out to the Mage owners in .NET team to see which of the proposed approach is feasible.

sujitnayak commented 2 years ago

@drauch The timestamp server URL that is provided to CertTimestampAuthenticodeLicense determines which hash algorithm to use. Can you clarify which timestamp server you are using. Thanks.

drauch commented 2 years ago

@sujitnayak : The problem is, that CertTimestampAuthenticodeLicense should never be called in the first place. We identified three reasons why ObtainRFC3161Timestamp can fail in our scenarios:

All three scenarios are handled by the existing code as "TSS does not understand RFC 3161 properly", but at least for those three scenarios that's simply not true. In the contrary, most, if not all current TSS understand RFC 3161.

There should be a) better detection what caused the problem and not an immediate fallback to non-RFC-3161 and only afterwards there should be b) a check that the applied non-RFC-3161 timestamp has the same hash algorithm as originally requested with the useSha256 flag.

Timestamp servers which return SHA1 hashes for non-RFC-3161 timestamps are, e.g., DigiCert: http://timestamp.digicert.com

Best regards. D.R.

sujitnayak commented 2 years ago

@drauch is it possible to use a TSS that does not use sha-1 hash? The Windows API used for RFC3161 timestamp does not return HTTP errors so we cannot make a distinction between the various HTTP errors. We cannot remove the fallback at this point as there might be consumers that depend on the behavior. We could add a build warning if msbuild has a way of doing that.

rainersigwald commented 2 years ago

We could add a build warning if msbuild has a way of doing that.

We do, and if you use a unique warning/error code, a project could opt into making it a hard error (even if it's just a warning in normal operation).

We generally think of adding warnings as a breaking change, but it's a case-by-case thing; if this situation is both fairly rare and bad we could probably add a non-opt-in warning; if it's common or not too severe we'd probably prefer to add a new property DisallowMansignTimestampFallback (or something).

drauch commented 2 years ago

@rainersigwald : That sounds good enough for us. Even opt-in is good enough for our scenario.

Thank you both for your work!

Best regards, D.R.