| Message ID | 20200609050947.17861-9-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: rework/improve UEFI secure boot code | expand |
On 09.06.20 07:09, AKASHI Takahiro wrote: > Since the size check against an entry in efi_search_siglist() is > incorrect, this function will never find out a to-be-matched certificate > and its associated revocation time in the signature list. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_signature.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > index a05c75472721..f22dc151971f 100644 > --- a/lib/efi_loader/efi_signature.c > +++ b/lib/efi_loader/efi_signature.c > @@ -434,10 +434,11 @@ static bool efi_search_siglist(struct x509_certificate *cert, > * time64_t revocation_time; > * }; > */ > - if ((sig_data->size == SHA256_SUM_LEN) && > - !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) { > + if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) && > + !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) { > memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN, > sizeof(*revoc_time)); > + EFI_PRINT("revocation time: %llu\n", *revoc_time); *revoc_time is of type __s64. So this must be %lld. Cf. include/linux/time.h:156 Best regards Heinrich > found = true; > goto out; > } >
On Fri, Jul 03, 2020 at 01:00:21PM +0200, Heinrich Schuchardt wrote: > On 09.06.20 07:09, AKASHI Takahiro wrote: > > Since the size check against an entry in efi_search_siglist() is > > incorrect, this function will never find out a to-be-matched certificate > > and its associated revocation time in the signature list. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_signature.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > > index a05c75472721..f22dc151971f 100644 > > --- a/lib/efi_loader/efi_signature.c > > +++ b/lib/efi_loader/efi_signature.c > > @@ -434,10 +434,11 @@ static bool efi_search_siglist(struct x509_certificate *cert, > > * time64_t revocation_time; > > * }; > > */ > > - if ((sig_data->size == SHA256_SUM_LEN) && > > - !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) { > > + if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) && > > + !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) { > > memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN, > > sizeof(*revoc_time)); > > + EFI_PRINT("revocation time: %llu\n", *revoc_time); > > *revoc_time is of type __s64. So this must be %lld. Cf. > > include/linux/time.h:156 I know that because I added the definition. Interestingly, linux added another type, timeu64_t, later on to avoid an overflow in some calculation. While I don't think the current format is harmful, I will change it. -Takahiro Akashi > Best regards > > Heinrich > > > found = true; > > goto out; > > } > > >
On Wed, Jul 08, 2020 at 10:12:38AM +0900, AKASHI Takahiro wrote: > On Fri, Jul 03, 2020 at 01:00:21PM +0200, Heinrich Schuchardt wrote: > > On 09.06.20 07:09, AKASHI Takahiro wrote: > > > Since the size check against an entry in efi_search_siglist() is > > > incorrect, this function will never find out a to-be-matched certificate > > > and its associated revocation time in the signature list. > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > lib/efi_loader/efi_signature.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > > > index a05c75472721..f22dc151971f 100644 > > > --- a/lib/efi_loader/efi_signature.c > > > +++ b/lib/efi_loader/efi_signature.c > > > @@ -434,10 +434,11 @@ static bool efi_search_siglist(struct x509_certificate *cert, > > > * time64_t revocation_time; > > > * }; > > > */ > > > - if ((sig_data->size == SHA256_SUM_LEN) && > > > - !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) { > > > + if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) && > > > + !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) { > > > memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN, > > > sizeof(*revoc_time)); > > > + EFI_PRINT("revocation time: %llu\n", *revoc_time); > > > > *revoc_time is of type __s64. So this must be %lld. Cf. > > > > include/linux/time.h:156 > > I know that because I added the definition. > Interestingly, linux added another type, timeu64_t, later on > to avoid an overflow in some calculation. > > While I don't think the current format is harmful, I will change it. Oops, I have already changed the format to "0x%llx" locally. I will stick to this. > -Takahiro Akashi > > > > Best regards > > > > Heinrich > > > > > found = true; > > > goto out; > > > } > > > > >
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c index a05c75472721..f22dc151971f 100644 --- a/lib/efi_loader/efi_signature.c +++ b/lib/efi_loader/efi_signature.c @@ -434,10 +434,11 @@ static bool efi_search_siglist(struct x509_certificate *cert, * time64_t revocation_time; * }; */ - if ((sig_data->size == SHA256_SUM_LEN) && - !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) { + if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) && + !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) { memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN, sizeof(*revoc_time)); + EFI_PRINT("revocation time: %llu\n", *revoc_time); found = true; goto out; }
Since the size check against an entry in efi_search_siglist() is incorrect, this function will never find out a to-be-matched certificate and its associated revocation time in the signature list. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_signature.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)