diff mbox series

crypt: Remove invalid end of page test badsalttest

Message ID 20230227123700.106092-1-adhemerval.zanella@linaro.org
State New
Headers show
Series crypt: Remove invalid end of page test badsalttest | expand

Commit Message

Adhemerval Zanella Feb. 27, 2023, 12:37 p.m. UTC 
The input argument passes an invalid string with a NULL terminator
on crypt settings inputs, which might lead to invalid OOB on strncmp
(where multiple implementation assumes that the input is always a
NULL terminated string).

Also adapt the code to use libsupport.

Checked on arm-linux-gnuabihf.
---
 crypt/badsalttest.c | 50 ++++++++-------------------------------------
 1 file changed, 9 insertions(+), 41 deletions(-)

Comments

Florian Weimer Feb. 27, 2023, 12:59 p.m. UTC | #1 
* Adhemerval Zanella:

> The input argument passes an invalid string with a NULL terminator

“null terminator” or “NUL terminator”

>    /* Check that crypt won't look at the second character if the first
>       one is invalid.  */
> -  page = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
> -	       MAP_PRIVATE | MAP_ANON, -1, 0);
> -  if (page == MAP_FAILED)
> -    {
> -      perror ("mmap");
> -      n--;
> -    }
> -  else
> -    {
> -      if (mmap (page + pagesize, pagesize, 0,
> -		MAP_PRIVATE | MAP_ANON | MAP_FIXED,
> -		-1, 0) != page + pagesize)
> -	perror ("mmap 2");
> -      page[pagesize - 1] = '*';
> -      tests[n - 1][1] = &page[pagesize - 1];
> -    }
> -
>    /* Mark cd as initialized before first call to crypt_r.  */
>    cd.initialized = 0;

Please delete the comment as well.

Thanks,
Florian
Wilco Dijkstra Feb. 27, 2023, 1:29 p.m. UTC | #2 
Hi Adhemerval,

> The input argument passes an invalid string with a NULL terminator

*without* a NUL terminator

> on crypt settings inputs, which might lead to invalid OOB on strncmp
> (where multiple implementation assumes that the input is always a
> NULL terminated string).

Implementations only assume there is a NUL terminator if the string is shorter
than the specified size, so strings don't need to always be NUL terminated
(stratcliff.c has tests for this).

Cheers,
Wilco
diff mbox series

Patch

diff --git a/crypt/badsalttest.c b/crypt/badsalttest.c
index bc1e5c1442..acf4ed1ad9 100644
--- a/crypt/badsalttest.c
+++ b/crypt/badsalttest.c
@@ -16,11 +16,12 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/mman.h>
+#include <array_length.h>
+#include <stddef.h>
 #include <crypt.h>
 
+#include <support/check.h>
+
 static const char *tests[][2] =
   {
     { "no salt", "" },
@@ -30,59 +31,26 @@  static const char *tests[][2] =
     { "both chars bad", ":@" },
     { "un$upported algorithm", "$2$" },
     { "unsupported_algorithm", "_1" },
-    { "end of page", NULL }
   };
 
 static int
 do_test (void)
 {
-  int result = 0;
   struct crypt_data cd;
-  size_t n = sizeof (tests) / sizeof (*tests);
-  size_t pagesize = (size_t) sysconf (_SC_PAGESIZE);
-  char *page;
 
   /* Check that crypt won't look at the second character if the first
      one is invalid.  */
-  page = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
-	       MAP_PRIVATE | MAP_ANON, -1, 0);
-  if (page == MAP_FAILED)
-    {
-      perror ("mmap");
-      n--;
-    }
-  else
-    {
-      if (mmap (page + pagesize, pagesize, 0,
-		MAP_PRIVATE | MAP_ANON | MAP_FIXED,
-		-1, 0) != page + pagesize)
-	perror ("mmap 2");
-      page[pagesize - 1] = '*';
-      tests[n - 1][1] = &page[pagesize - 1];
-    }
-
   /* Mark cd as initialized before first call to crypt_r.  */
   cd.initialized = 0;
 
-  for (size_t i = 0; i < n; i++)
+  for (size_t i = 0; i < array_length (tests); i++)
     {
-      if (crypt (tests[i][0], tests[i][1]))
-	{
-	  result++;
-	  printf ("%s: crypt returned non-NULL with salt \"%s\"\n",
-		  tests[i][0], tests[i][1]);
-	}
+      TEST_VERIFY (crypt (tests[i][0], tests[i][1]) == NULL);
 
-      if (crypt_r (tests[i][0], tests[i][1], &cd))
-	{
-	  result++;
-	  printf ("%s: crypt_r returned non-NULL with salt \"%s\"\n",
-		  tests[i][0], tests[i][1]);
-	}
+      TEST_VERIFY (crypt_r (tests[i][0], tests[i][1], &cd) == NULL);
     }
 
-  return result;
+  return 0;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>