Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pj_gridinfo_init does not check the read size of header before doing string accesses #875

Closed
schwehr opened this issue Mar 20, 2018 · 4 comments
Milestone

Comments

@schwehr
Copy link
Contributor

@schwehr schwehr commented Mar 20, 2018

From MSAN fuzzing:

third_party/proj4/tests/corpus/poc/poc-31059c2fd8a29418e617c553051285fbbe8bda00e1cd4f78644479d5c3fc11da (196 bytes)
Uninitialized bytes in __interceptor_strncmp at offset 0 inside [0x7ffd779b7bc0, 1)
==8165==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5561ee5f651b in pj_gridinfo_init third_party/proj4/proj/src/pj_gridinfo.c:911:9
    #1 0x5561ee5cfd3e in pj_gridlist_merge_gridfile third_party/proj4/proj/src/pj_gridlist.c:126:17
    #2 0x5561ee5cf8b6 in pj_gridlist_from_nadgrids third_party/proj4/proj/src/pj_gridlist.c:202:14
    #3 0x5561ee6054f8 in pj_apply_gridshift_2 third_party/proj4/proj/src/pj_apply_gridshift.c:94:13
    #4 0x5561ee5e669c in pj_datum_transform third_party/proj4/proj/src/pj_transform.c:906:9
    #5 0x5561ee5e4762 in datum_transform third_party/proj4/proj/src/pj_transform.c:438:12
    #6 0x5561ee5e31d6 in pj_transform third_party/proj4/proj/src/pj_transform.c:527:11
    #7 0x5561ee008962 in TestOneProtoInput(third_party::proj4::tests::Proj4 const&) third_party/proj4/tests/proj4_fuzzer.cc:62:3
    #8 0x5561ee0085ef in LLVMFuzzerTestOneInput third_party/proj4/tests/proj4_fuzzer.cc:48:1

Proposed fix:

--- b/src/pj_gridinfo.c	2018-03-19 09:03:14.000000000 -0700
+++ a/src/pj_gridinfo.c	2018-03-20 10:28:03.000000000 -0700
@@ -850,7 +850,9 @@
     char 	fname[MAX_PATH_FILENAME+1];
     PJ_GRIDINFO *gilist;
     PAFile 	fp;
-    char	header[160];
+#define MAX_HEADER 160
+    char	header[MAX_HEADER];
+    size_t header_size = 0;
 
     errno = pj_errno = 0;
     ctx->last_errno = 0;
@@ -897,25 +899,31 @@
 /* -------------------------------------------------------------------- */
 /*      Load a header, to determine the file type.                      */
 /* -------------------------------------------------------------------- */
-    if( pj_ctx_fread( ctx, header, sizeof(header), 1, fp ) != 1 )
+    if( (header_size = pj_ctx_fread( ctx, header, 1,
+                                     MAX_HEADER, fp ) ) != MAX_HEADER )
     {
         /* some files may be smaller that sizeof(header), eg 160, so */
         ctx->last_errno = 0; /* don't treat as a persistent error */
+        pj_log( ctx, PJ_LOG_DEBUG_MAJOR,
+                "pj_gridinfo_init: short header read of %d bytes",
+                (int)header_size );
     }
-
     pj_ctx_fseek( ctx, fp, SEEK_SET, 0 );
 
 /* -------------------------------------------------------------------- */
 /*      Determine file type.                                            */
 /* -------------------------------------------------------------------- */
-    if( strncmp(header + 0, "HEADER", 6) == 0
+    // TODO(schwehr): Bare literals are hard to follow.
+    if( header_size >= 144 + 16
+        && strncmp(header + 0, "HEADER", 6) == 0
         && strncmp(header + 96, "W GRID", 6) == 0
         && strncmp(header + 144, "TO      NAD83   ", 16) == 0 )
     {
         pj_gridinfo_init_ntv1( ctx, fp, gilist );
     }
 
-    else if( strncmp(header + 0, "NUM_OREC", 8) == 0
+    else if( header_size >= 48 + 7
+             && strncmp(header + 0, "NUM_OREC", 8) == 0
              && strncmp(header + 48, "GS_TYPE", 7) == 0 )
     {
         pj_gridinfo_init_ntv2( ctx, fp, gilist );
@@ -928,7 +936,7 @@
         pj_gridinfo_init_gtx( ctx, fp, gilist );
     }
 
-    else if( strncmp(header + 0,"CTABLE V2",9) == 0 )
+    else if( header_size >= 9 && strncmp(header + 0,"CTABLE V2",9) == 0 )
     {
         struct CTABLE *ct = nad_ctable2_init( ctx, fp );
 

With setenv("PROJ_DEBUG", "2", 1), it gives:

third_party/proj4/tests/proj4_fuzzer third_party/proj4/tests/corpus/poc/poc-31059c2fd8a29418e617c553051285fbbe8bda00e1cd4f78644479d5c3fc11da

Running: third_party/proj4/tests/corpus/poc/poc-31059c2fd8a29418e617c553051285fbbe8bda00e1cd4f78644479d5c3fc11da
pj_ellipsoid - final: a=6378137.000 f=1/298.257, errno=0
pj_ellipsoid - final:    ellps=WGS84
pj_ellipsoid - final: a=6378137.000 f=1/298.257, errno=0
pj_ellipsoid - final:    ellps=WGS84
pj_open_lib(proj_def.dat): call fopen(proj_def.dat) - succeeded
pj_open_lib(proj_def.dat): call fopen(proj_def.dat) - succeeded
pj_ellipsoid - final: a=6378137.000 f=1/298.257, errno=0
pj_ellipsoid - final:    ellps=WGS84
pj_open_lib(proj_def.dat): call fopen(proj_def.dat) - succeeded
pj_ellipsoid - final: a=6378137.000 f=1/298.257, errno=0
pj_ellipsoid - final:    ellps=WGS84
pj_open_lib(/): call fopen(/) - succeeded
pj_gridinfo_init: short header read of 0 bytes
CTABLE ct is NULL.
Executed third_party/proj4/tests/corpus/poc/poc-31059c2fd8a29418e617c553051285fbbe8bda00e1cd4f78644479d5c3fc11da in 5 ms
@schwehr
Copy link
Contributor Author

@schwehr schwehr commented Mar 20, 2018

BTW, I find all these magic number very hard to follow. Symbolic constants and/or some comments would really help with the readability.

@schwehr
Copy link
Contributor Author

@schwehr schwehr commented Mar 20, 2018

Alternatively or in combination, you could zero initialize the header buffer with a memset before the read.

@rouault
Copy link
Member

@rouault rouault commented Mar 20, 2018

@schwehr Please submit a pull request instead of a inline patch

schwehr added a commit to schwehr/PROJ that referenced this issue Mar 20, 2018
Fixes OSGeo#875

Found with autofuzz using MSAN: use-of-uninitialized-value
@schwehr
Copy link
Contributor Author

@schwehr schwehr commented Mar 20, 2018

Decided to stick with sizeof rather than MAX_HEADER

@kbevers kbevers closed this in #876 Mar 20, 2018
kbevers added a commit that referenced this issue Mar 20, 2018
Fixes #875

Found with autofuzz using MSAN: use-of-uninitialized-value
@kbevers kbevers added this to the 5.0.1 milestone Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.