smallstep/pkcs7

verifySignatureAtTime does not use currentTime

ch-v2s opened this issue ยท 11 comments

I was wondering, why is the verifySignatureAtTime function not using the supplied 'currentTime' parameter to check the validity of the signature header?

My understanding was that this function was introduced to handle a certain clock skew. But it seems like it is taking a new timestamp 'signingTime' each time and check it against the notBefore/notAfter.

I have this issue when fiddling around with the "-device-signature-skew " parameter in micromdm.

-cheers

Did a quick skim, but will need a bit more time to check the different code paths.

@ch-v2s curious, have you noticed this behavior with specific device OSes? I.e. iOS vs. macOS?

@ch-v2s curious, have you noticed this behavior with specific device OSes? I.e. iOS vs. macOS?

@jessepeterson indeed! But I figured only just now.
Right now I have three testing devices and only a tvOS is getting the error:
Apple TV 4th G 2GB | 29.6GB | tvOS 17.3 (21K646)
The others are macOS (M1/Ventura 13.6.1 && i3/BigSur 11.7.10)

Looking further, it seems that only the tvOS is sending a timestamp within the Mdm-Signature Payload?

They do have different User-Agent Data:
User-Agent: MDM-OSX/1.0 mdmclient/1660 (macOS 13.6.3 M1)
User-Agent: MDM-OSX/1.0 mdmclient/1455 (macOS 11.7.10 i3)
User-Agent: MDM/1.0 (tvOS 17.3)

Looking further, it seems that only the tvOS is sending a timestamp within the Mdm-Signature Payload?

@ch-v2s Ahh, interesting; thanks for that! I was wondering why I wasn't able to duplicate this on macOS.

Hey @ch-v2s, the signingTime you're referring to seems to be the timestamp in one of the signed attributes, which contains the time of signing the message. That value is extracted, and then verified against the validity of the certificate for the public key belonging to the private key that signed the message to verify it's within its bounds. I think that's the right logic for that part of the verification.

The currentTime is in fact passed down to verifyCertChain and used there to verify if the certificate chain was valid at the provided time. This is similar to how you would verify a signed software artifact with a certificate (chain) that is expired, but was valid at the time of the signature, assuming there's trusted timestamp.

So, if you're seeing failures like pkcs7: signing time ... is outside of certificate validity ... to ..., check that the signed attribute(s) reflect the right time, and that it's within the validity of the certificate.

@hslatman thanks for looking into this and for clarification.
It makes sense to me now - I wasn't aware that attributes are signed as well and that this is the issue here.
From the technical aspect the actual behaviour seems correct.

However, the fact that tvOS is signing its attributes and macOS doesn't ist still irritating :)

So, if you're seeing failures like pkcs7: signing time ... is outside of certificate validity ... to ..., check that the signed attribute(s) reflect the right time, and that it's within the validity of the certificate.

I think part of this issue is that we'd like to see an ability to specify a skew, even for this signing time vs. certificate validity (or disable that altogether and pretend the signing time attribute isn't there). One way to at least address this would be implement a custom error that we can catch and handle (with e.g. validity and sign-time attributes we can decide on ourselves, perhaps) rather than being a generic "hard" error.

I think the underlying problem, for MDM enrollment anyway, is that the device's time may be wrong โ€” but the certificate itself is fine (because it was issued from a SCEP server with correct time). So the sign time attribute does not fall within the validity of the certificate.

And when an Apple device fails to enroll due to a SCEP error it is very opaque to the end-user and troublesome to find the cause. So, perhaps with an ability to allow some skew for this as well as catch and handle this specific error we may be able to strategize better ways of informing the user in a nicer way.

cc @korylprince

Yes, totally agree, @jessepeterson ๐Ÿ™‚

A typed error sounds like a good approach to me for this case.

Entirely disabling the time check (conditionally) is the simplest for the user, but that would be a slightly different pattern than being able to provide a time for this check (as is the case for the certificate chain validity check). I think the combination of a typed error, with a clear error message, and being able to provide a time, is a good compromise over entirely disabling the check.

friendly ping @hslatman :)

@jessepeterson, @ch-v2s I've just merged #25. I'll follow up with a test for the new error "behavior".

Edit: PR with a new test case: #26

Closing this issue now as completed. With @jessepeterson's change it's now possible to handle this specific error case in relying code in an application-specific manner using e.g. errors.As.