[edk2-devel] [PATCH] CryptoPkg: Fix BaseCryptLib CrtWrapper strcpy

[edk2-devel] [PATCH] CryptoPkg: Fix BaseCryptLib CrtWrapper strcpy

[Witt, Sebastian via groups.io]

strcpy fails when strSource is closer than 4096 bytes after strDest.

This is caused by an overlap check in AsciiStrCpyS:
  //
  // 5. Copying shall not take place between objects that overlap.
  //
  SAFE_STRING_CONSTRAINT_CHECK (InternalSafeStringNoAsciiStrOverlap (Destination, DestMax, (CHAR8 *)Source, SourceLen + 1), RETURN_ACCESS_DENIED);

Since DestMax is MAX_STRING_SIZE (0x1000) and with a Source
that is in this area behind Destination, AsciiStrCpyS will fail
and strcpy will do nothing.

When called by CRYPTO_strdup in openssl this leads to uninitialzed
memory that gets accessed instead of the copied string.

Signed-of-by: Sebastian Witt <sebastian.witt@siemens.com>
---
 CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
index 37cdecc9bd..880ed140fd 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
@@ -271,7 +271,7 @@ strcpy (
   const char  *strSource
   )
 {
-  AsciiStrCpyS (strDest, MAX_STRING_SIZE, strSource);
+  AsciiStrCpyS (strDest, AsciiStrnSizeS (strSource, MAX_STRING_SIZE), strSource);
   return strDest;
 }
 
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119436): https://edk2.groups.io/g/devel/message/119436
Mute This Topic: https://groups.io/mt/106471263/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] CryptoPkg: Fix BaseCryptLib CrtWrapper strcpy

[Li, Yi]

Thanks for your patch, this is a known issue: https://bugzilla.tianocore.org/show_bug.cgi?id=2817

Could you also update other wrappers in CrtWrapper.h and add BZ link to commit message?

Edk2 has switched to github pr code review process, you can raise PR in https://github.com/tianocore/edk2/pulls directly.

Regards,
Yi

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Witt, Sebastian via groups.io
Sent: Tuesday, June 4, 2024 12:19 AM
To: devel@edk2.groups.io
Subject: [edk2-devel] [PATCH] CryptoPkg: Fix BaseCryptLib CrtWrapper strcpy


strcpy fails when strSource is closer than 4096 bytes after strDest.

This is caused by an overlap check in AsciiStrCpyS:
  //
  // 5. Copying shall not take place between objects that overlap.
  //
  SAFE_STRING_CONSTRAINT_CHECK (InternalSafeStringNoAsciiStrOverlap (Destination, DestMax, (CHAR8 *)Source, SourceLen + 1), RETURN_ACCESS_DENIED);

Since DestMax is MAX_STRING_SIZE (0x1000) and with a Source that is in this area behind Destination, AsciiStrCpyS will fail and strcpy will do nothing.

When called by CRYPTO_strdup in openssl this leads to uninitialzed memory that gets accessed instead of the copied string.

Signed-of-by: Sebastian Witt <sebastian.witt@siemens.com>
---
 CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
index 37cdecc9bd..880ed140fd 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
@@ -271,7 +271,7 @@ strcpy (
   const char  *strSource
   )
 {
-  AsciiStrCpyS (strDest, MAX_STRING_SIZE, strSource);
+  AsciiStrCpyS (strDest, AsciiStrnSizeS (strSource, MAX_STRING_SIZE), 
+ strSource);
   return strDest;
 }
 
--
2.39.2








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119443): https://edk2.groups.io/g/devel/message/119443
Mute This Topic: https://groups.io/mt/106471263/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] CryptoPkg: Fix BaseCryptLib CrtWrapper strcpy

[Witt, Sebastian via groups.io]

Ok thanks, I'll look at the other wrappers and do a PR.

Regards,
Sebastian

On Tue, 2024-06-04 at 07:28 +0000, Li, Yi1 wrote:
> Thanks for your patch, this is a known issue:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2817
>
> Could you also update other wrappers in CrtWrapper.h and add BZ link to commit message?
>
> Edk2 has switched to github pr code review process, you can raise PR in
> https://github.com/tianocore/edk2/pulls
>  directly.
>
> Regards,
> Yi
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Witt, Sebastian via groups.io
> Sent: Tuesday, June 4, 2024 12:19 AM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH] CryptoPkg: Fix BaseCryptLib CrtWrapper strcpy
>
>
> strcpy fails when strSource is closer than 4096 bytes after strDest.
>
> This is caused by an overlap check in AsciiStrCpyS:
>   //
>   // 5. Copying shall not take place between objects that overlap.
>   //
>   SAFE_STRING_CONSTRAINT_CHECK (InternalSafeStringNoAsciiStrOverlap (Destination, DestMax, (CHAR8 *)Source, SourceLen + 1), RETURN_ACCESS_DENIED);
>
> Since DestMax is MAX_STRING_SIZE (0x1000) and with a Source that is in this area behind Destination, AsciiStrCpyS will fail and strcpy will do nothing.
>
> When called by CRYPTO_strdup in openssl this leads to uninitialzed memory that gets accessed instead of the copied string.
>
> Signed-of-by: Sebastian Witt <sebastian.witt@siemens.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> index 37cdecc9bd..880ed140fd 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> @@ -271,7 +271,7 @@ strcpy (
>    const char  *strSource
>    )
>  {
> -  AsciiStrCpyS (strDest, MAX_STRING_SIZE, strSource);
> +  AsciiStrCpyS (strDest, AsciiStrnSizeS (strSource, MAX_STRING_SIZE),
> + strSource);
>    return strDest;
>  }
>
> --
> 2.39.2
>
>
>
> 
>
>

--
Sebastian Witt
Siemens AG
http://www.siemens.com/


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119444): https://edk2.groups.io/g/devel/message/119444
Mute This Topic: https://groups.io/mt/106471263/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-