apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Pane <bp...@pacbell.net>
Subject [PATCH] apr_filepath_merge()
Date Sat, 08 Jun 2002 05:52:31 GMT
apr_filepath_merge() contains some convoluted code to ensure
that it doesn't overflow the buffer that it has allocated.

The current semantics are basically: If it looks like the
merged string is going to be too long, but the root component
of the path is shorter than APR_PATH_MAX, then go ahead and
start merging the paths, just in case there are enough
"/foo/../bar/" constructs to make the result short enough
to fit within APR_PATH_MAX characters; but while doing this
merging, check for a buffer overflow at every point where
we append to the target string.

IMO, the fact that this algorithm needs to check for overflow
at so many spots is an accident waiting to happen.  As the code
evolves in the future, all it takes is one missed boundary case
to render us vulnerable to a buffer overwrite.

The following patch replaces the current logic with a single
check at the beginning: if there is any chance that the merged
string will exceed APR_PATH_MAX characters (because the sum
of the strings being merged exceeds APR_PATH_MAX, then it
returns an error.

The downside of the patch is that ridiculous use cases like
  apr_filepath_merge(&new_path, "/root/path",
                     "/foo/../bar/../foo/../bar..[etc, etc.]../path",
                     flags, pool)
won't work any more if the path being appended is longer than
APR_PATH_MAX but contains enough "/../" sequences to reduce
the total length to <= APR_PATH_MAX.

If anyone has a real-world example that depends on the current
semantics, please let me know.  If not, I'll commit in a few days.

Thanks,
--Brian


Mime
View raw message