TPM2: Added check when parsing TPMLPCRSelection in case of selection of a PCR's index which exceeds max value permitted #204
Conversation
…of a PCR's index which exceeds 8*sizeOfPCRSelect-1 to avoid panic runtime error The current implementation makes use of `sizeOfPCRSelect` set to `3`. This means that the maximum size of the `pcrSelect` byte array in `tpmsPCRSelection` is `3`, which means it can only deal with the selection of PCRs from 0 to 23. Attempting to select PCR 24 for example (which might exist on some TPMs) leads to a 'panic: runtime error: index out of range [3] with length 3' since `ts.PCRs[byteNum] |= bytePos` tries to access index 3 of the byte array, which is out of range. This can be fixed by adding a check beforehand, and skipping the PCR if it's index exceeds the maximum value permitted, independently of the value of `sizeOfPCRSelect`. Signed-off-by: El Mostafa IDRASSI <[email protected]>
ElMostafaIdrassi
requested review from
josephlr and
twitchy-jsonp
as code owners
|
Have you seen a TPM with more than 24 PCRs in the wild? We should definitely revise PCR selection generation code if the PCR 0-23 assumption is no longer correct. I think the spec permits more than 24 PCRs, but I've never heard of any device doing something like that (and no use-case for additional PCRs). |
|
@twitchy-jsonp Nope, I've never seen a TPM with more than 24 PCRs. I think the spec specifies the minimum number of PCRs to be 24. However, I find it to be safer to add the check and skip all the PCRs that currently cannot be selected and avoid a panic error. |
|
Thats a good point. What do you think about returning an error from |
|
I believe the problem stems more from feeding For example, the function will do just fine if you give it the following 25 PCRs This is why I believe the check should be done on an index-basis rather than a length one. Concerning how to deal with it, I'm totally fine with returning an error instead of just skipping. |
|
I agree that the check should be done per index and an error should be returned instead of just skipping. |
|
Yepp that makes sense! |
|
@ElMostafaIdrassi, can you go ahead and implement these changes? |
Signed-off-by: El Mostafa IDRASSI <[email protected]>
The current implementation makes use of
sizeOfPCRSelectset to3. This means that the maximum size of thepcrSelectbyte array intpmsPCRSelectionis3, which means it can only deal with the selection of PCRs from 0 to 23. Attempting to select PCR 24 for example (which might exist on some TPMs) leads to apanic: runtime error: index out of range [3] with length 3sincets.PCRs[byteNum] |= bytePostries to access index 3 of the byte array, which is out of range.This can be fixed by adding a check beforehand, and skipping the PCR if it's index exceeds the maximum value permitted, independently of the value of
sizeOfPCRSelect.Signed-off-by: El Mostafa IDRASSI [email protected]