Potential buffer overflow in geotag.cpp on non-glibc systems

[RootUp]

File: geotag.cpp

#if defined(_MSC_VER) || defined(__MINGW__)
#include <windows.h>
char*    realpath(const char* file,char* path); 
#define  lstat _stat
#define  stat  _stat
#if      _MSC_VER < 1400
#define strcpy_s(d,l,s) strcpy(d,s)
#define strcat_s(d,l,s) strcat(d,s)
#endif
#endif

and

#ifdef __APPLE__
                    char   buffer[1024];
#else
                    char*  buffer = NULL;
#endif
                    char*  path = realpath(arg,buffer);
                    if  ( t && path ) {
                        if ( options.verbose) printf("%s %ld %s",path,(long int)t,asctime(localtime(&t)));
                        gFiles.push_back(path);
                    }
                    if ( path && path != buffer ) ::free((void*) path);
                }
                if ( type == typeUnknown ) {
                    fprintf(stderr,"error: illegal syntax %s\n",arg);
                    result = resultSyntaxError ;
                }
            } break;
        }
}

According to the documentation of realpath() the output buffer needs to be at least of size PATH_MAX specifying output buffers large enough to handle the maximum-size possible result from path manipulation functions. In that instance, buf's size comes from uv__fs_pathmax_size(). That function attempts to use pathconf(path, _PC_PATH_MAX) as noted in the realpath(3) docs.

But over here uv__fs_pathmax_size() nor pathconf(path, _PC_PATH_MAX) is used.

Passing an inadequately-sized output buffer to a path manipulation function can result in a buffer overflow. Such functions include realpath() readlink() PathAppend() and others.

[clanmills]

Thank you for reporting this. Do you have a proposed fix?

I'm lost in your explanation. You'll be aware that realpath() is platform dependent. Are you talking about Linux? samples/geotag.cpp is sample code and we have a test in our test suite for that code. So the code is working on the supported platforms.

Do you have a test case which fails, or is this a "potential" vulnerability? Or are you saying that we should be calling pathconf() on Linux?

[RootUp]

Thank for taking look into this,
Yes this I am talking about Linux and this is a potential vulnerability, however I think calling pathconf() on Linux would help or PATH_MAX+1

[clanmills]

Gosh, you're quick! Thanks for such a speedy response. I haven't thought about this matter for years, however here's a way of testing it.

1034 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ cp ../test/data/FurnaceCreekInn* /tmp
1035 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -pa --grep GPSLongitude /tmp/FurnaceCreekInn.jpg 
Exif.GPSInfo.GPSLongitudeRef                 Ascii       2  West
Exif.GPSInfo.GPSLongitude                    Rational    3  116deg 51' 18" 
1036 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -M'set Exif.GPSInfo.GPSLongitude 1/1 2/1 3/1' /tmp/FurnaceCreekInn.jpg 
1037 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -pa --grep GPSLongitude /tmp/FurnaceCreekInn.jpg 
Exif.GPSInfo.GPSLongitudeRef                 Ascii       2  West
Exif.GPSInfo.GPSLongitude                    Rational    3  1deg 2' 3" 
1038 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ bin/geotag -ascii -tz -8.00 /tmp/FurnaceCreekInn.jpg /tmp/FurnaceCreekInn.gpx 
/tmp/FurnaceCreekInn.jpg 116deg51'18"W 036deg26'54"N -14.282  -3
1039 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -pa --grep GPSLongitude /tmp/FurnaceCreekInn.jpg 
Exif.GPSInfo.GPSLongitudeRef                 Ascii       2  West
Exif.GPSInfo.GPSLongitude                    Rational    3  116deg 51' 18" 
1040 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ 

I've modified the code where realpath() is used as follows:

                    time_t t    = readImageTime(std::string(arg)) ;
#if   defined __APPLE__
                    char   buffer[1024];
#elif defined(__gnu_linux__)
                    char   buffer[_MAX_PATH];
                    std::cout << "realpath: " << arg << " _MAX_PATH  = " << _MAX_PATH << std::endl;
                    pathconf(arg,_MAX_PATH);
#else
                    char*  buffer = NULL;
#endif
                    char*  path = realpath(arg,buffer);
#if defined(__gnu_linux__)
                    std::cout << "realpath: " << path << std::endl;
#endif
                    if  ( t && path ) {

To test this:

1070 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ mkdir /tmp/testing-directory
1071 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ bin/geotag -ascii -tz -8.00 /tmp/testing-directory/../FurnaceCreekInn.jpg /tmp/testing-directory/../FurnaceCreekInn.gpx  
realpath: /tmp/testing-directory/../FurnaceCreekInn.jpg _MAX_PATH  = 1024
path: /tmp/FurnaceCreekInn.jpg
/tmp/FurnaceCreekInn.jpg 116deg51'18"W 036deg26'54"N -14.282  -3
1072 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ 

Is that what you are thinking?

[RootUp]

Yup, the modified code for realpath() above looks perfect to me.

[clanmills]

Patch:

1102 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ git diff
diff --git a/samples/geotag.cpp b/samples/geotag.cpp
index 1355827..4da38af 100644
--- a/samples/geotag.cpp
+++ b/samples/geotag.cpp
@@ -806,8 +806,11 @@ int main(int argc,const char* argv[])
                 if ( options.verbose ) printf("%s %s ",arg,types[type]) ;
                 if ( type == typeImage ) {
                     time_t t    = readImageTime(std::string(arg)) ;
-#ifdef __APPLE__
+#if   defined(__APPLE__)
                     char   buffer[1024];
+#elif defined(__gnu_linux__)
+                    char   buffer[_MAX_PATH];
+                    pathconf(arg ,_MAX_PATH);
 #else
                     char*  buffer = NULL;
 #endif
1103 rmills@rmillsmm-kubuntu:~/gnu/github/exiv2/exiv2/build $ 

@D4N Can you review and submit this for me please?

[D4N]

Sorry, I'm a little late. To be honest I don't see the problem with the usage of realpath in the above code snippet on Linux. The manpage says:

     If resolved_path is specified  as NULL, then realpath() uses
     malloc(3) to  allocate a buffer  of up to PATH_MAX  bytes to
     hold the  resolved pathname, and  returns a pointer  to this
     buffer.   The caller  should  deallocate  this buffer  using
     free(3).

So in my eyes the code does exactly what it should be doing on Linux + glibc and it shouldn't create any issues. That is further indicated by the existence of glibc's canonicalize_file_name (see man 3 canonicalize_file_name, it's literally a call to realpath(path, NULL)).

I believe however that the behavior is definitely broken on POSIX systems where the libc does not malloc the path if resolved_path is NULL (which it can do, as POSIX says that behavior is undefined). We'll probably have to work around that. Were you referering to that issue @RootUp?

@clanmills The proposed patch actually makes the #ifdef obsolete, since it now declares buffer as char[1024] (_MAX_PATH is a Windows thing and not defined on *nix, thus it'll be set to 1024 in line 58 in geotag.cpp). You'd have to call pathconf before malloc'ing buffer, but I believe that glibc does that for you in realpath. That also explains why the testsuite breaks, as now the call to realpath overflows buffer.

[RootUp]

I found that overflow here,

#ifdef __APPLE__
                    char   buffer[1024];
#else
                    char*  buffer = NULL;
#endif
                    char*  path = realpath(arg,buffer);
                    if  ( t && path ) {
                        if ( options.verbose) printf("%s %ld %s",path,(long int)t,asctime(localtime(&t)));
                        gFiles.push_back(path);
                    }
                    if ( path && path != buffer ) ::free((void*) path);
                }
                if ( type == typeUnknown ) {
                    fprintf(stderr,"error: illegal syntax %s\n",arg);
                    result = resultSyntaxError ;
                }
            } break;
        }
}

I believe over her resolved_path was not specified as NULL

[clanmills]

@RootUp Please provide a command-line that produces an overflow.

@D4N You're quite right that _MAX_PATH is an MSVC thing and defined (without reason) for Linux in geotag.cpp.

When I looked at this on Sunday, I thought "I really don't know what's being discussed here, however perhaps we should call pathconf() before realpath() on Linux. In conversation with @RootUp, it does seem that this issue is based on a discussion of man pages and described as a "potential vulnerability".

So gentlemen: can we determine and focus on a solid issue. How do we reproduce the overflow?

[D4N]

@RootUp resolved_path is non-NULL when __APPLE__ is defined, since we have to assume that on MacOS you are not using glibc and therefore passing NULL to realpath is undefined behavior.

That is however potentially problematic if you have a path that is longer than 1024 bytes. So we have to fix this on POSIX systems that are not using glibc.

[piponazo]

I agree with the analysis done by @D4N here. Anyways, note that this file is a sample application, and maybe we should not get paranoid about it. If the code would have been in the library code I would not say the same, but in a sample application I could live with this situation 😺 .

[piponazo]

Since this has been opened for few months without taking any action about it, I'm going to close it.

@RootUp feel free to reopen it if you find an example of how to reproduce the issue.