[pacman-dev] [PATCH] validate %FILEPATH% when parsing repo dbs
Currently we make no effort to validate the %FILENAME% field in the repo db. This allows for relative paths to be considered valid. A carefully crafted db entry with a malicious relative path, (e.g. `../../../../etc/passwd`) will cause pacman to to overwrite _any_ file on the target's machine. Signed-off-by: Simon Gomizelj <simongmzlj@gmail.com> --- lib/libalpm/be_sync.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 1cbe055..059c18a 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -479,6 +479,34 @@ cleanup: return count; } +/* This function validates %FILENAME%. filename must be between 3 and + * PATH_MAX characters and cannot be contain a path */ +static int _alpm_validate_filename(alpm_db_t *db, const char *pkgname, + const char *filename) +{ + size_t len = strlen(filename); + + if(len < 3) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too short\n"), db->treename, pkgname); + return -1; + } else if(len > PATH_MAX) { + errno = EINVAL; + db->handle-> + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too long\n"), db->treename, pkgname); + return -1; + } else if(memchr(filename, '/', len) != NULL) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is illegal\n"), db->treename, pkgname); + return -1; + } + + return 0; +} + #define READ_NEXT() do { \ if(_alpm_archive_fgets(archive, &buf) != ARCHIVE_OK) goto error; \ line = buf.line; \ @@ -558,6 +586,9 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, } } else if(strcmp(line, "%FILENAME%") == 0) { READ_AND_STORE(pkg->filename); + if(_alpm_validate_filename(db, pkg->name, pkg->filename) < 0) { + return -1; + } } else if(strcmp(line, "%DESC%") == 0) { READ_AND_STORE(pkg->desc); } else if(strcmp(line, "%GROUPS%") == 0) { -- 1.8.2.2
On 09/05/13 12:05, Simon Gomizelj wrote:
Currently we make no effort to validate the %FILENAME% field in the repo db. This allows for relative paths to be considered valid.
A carefully crafted db entry with a malicious relative path, (e.g. `../../../../etc/passwd`) will cause pacman to to overwrite _any_ file on the target's machine.
Signed-off-by: Simon Gomizelj <simongmzlj@gmail.com> --- lib/libalpm/be_sync.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 1cbe055..059c18a 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -479,6 +479,34 @@ cleanup: return count; }
+/* This function validates %FILENAME%. filename must be between 3 and + * PATH_MAX characters and cannot be contain a path */
I still do not understand the lower bound of 3. Surely any file name is fine - pacman/libarchive does not use the file name to determine compression. The file name also needs to be less than: PATH_MAX - strlen(dbpath) - strlen(rootpath) or whatever those variables are called... the root might already be in dbpath.
+static int _alpm_validate_filename(alpm_db_t *db, const char *pkgname, + const char *filename) +{ + size_t len = strlen(filename); + + if(len < 3) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too short\n"), db->treename, pkgname); + return -1; + } else if(len > PATH_MAX) { + errno = EINVAL; + db->handle-> + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too long\n"), db->treename, pkgname); + return -1; + } else if(memchr(filename, '/', len) != NULL) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is illegal\n"), db->treename, pkgname);
Why "database is inconsistent"? That does not really correspond to the error we are checking.
+ return -1; + } + + return 0; +} + #define READ_NEXT() do { \ if(_alpm_archive_fgets(archive, &buf) != ARCHIVE_OK) goto error; \ line = buf.line; \ @@ -558,6 +586,9 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, } } else if(strcmp(line, "%FILENAME%") == 0) { READ_AND_STORE(pkg->filename); + if(_alpm_validate_filename(db, pkg->name, pkg->filename) < 0) { + return -1; + } } else if(strcmp(line, "%DESC%") == 0) { READ_AND_STORE(pkg->desc); } else if(strcmp(line, "%GROUPS%") == 0) {
Currently we make no effort to validate the %FILENAME% field in the repo db. This allows for relative paths to be considered valid. A carefully crafted db entry with a malicious relative path, (e.g. `../../../../etc/passwd`) will cause pacman to to overwrite _any_ file on the target's machine. Signed-off-by: Simon Gomizelj <simongmzlj@gmail.com> --- lib/libalpm/be_sync.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 1cbe055..2cd756d 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -479,6 +479,34 @@ cleanup: return count; } +/* This function validates %FILENAME%. filename must be between 3 and + * PATH_MAX characters and cannot be contain a path */ +static int _alpm_validate_filename(alpm_db_t *db, const char *pkgname, + const char *filename) +{ + size_t len = strlen(filename); + size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root); + + if(len < 3) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too short\n"), db->treename, pkgname); + return -1; + } else if(len - cache_len > PATH_MAX) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too long\n"), db->treename, pkgname); + return -1; + } else if(memchr(filename, '/', len) != NULL) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is illegal\n"), db->treename, pkgname); + return -1; + } + + return 0; +} + #define READ_NEXT() do { \ if(_alpm_archive_fgets(archive, &buf) != ARCHIVE_OK) goto error; \ line = buf.line; \ @@ -558,6 +586,9 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, } } else if(strcmp(line, "%FILENAME%") == 0) { READ_AND_STORE(pkg->filename); + if(_alpm_validate_filename(db, pkg->name, pkg->filename) < 0) { + return -1; + } } else if(strcmp(line, "%DESC%") == 0) { READ_AND_STORE(pkg->desc); } else if(strcmp(line, "%GROUPS%") == 0) { -- 1.8.2.2
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root); Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman. I'll leave the min length for now.
On 09/05/13 16:40, Simon Gomizelj wrote:
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root);
Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman.
I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension? Allan
On 09/05/13 16:48, Allan McRae wrote:
On 09/05/13 16:40, Simon Gomizelj wrote:
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root);
Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman.
I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension?
Discussed on IRC. I'd prefer to explicitly check for "." and ".." rather than have the restriction of three. Allan
Prevent scriptlets from clobbering pacman.log because they don't end their outputs with a newline. --- lib/libalpm/util.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 033058a..c1fd0bb 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -555,23 +555,34 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) exit(1); } else { /* this code runs for the parent only (wait on the child) */ - int status; - FILE *pipe_file; + int status, need_newline = 0; CLOSE(pipefd[1]); - pipe_file = fdopen(pipefd[0], "r"); - if(pipe_file == NULL) { - CLOSE(pipefd[0]); - retval = 1; - } else { - while(!feof(pipe_file)) { - char line[PATH_MAX]; - if(fgets(line, PATH_MAX, pipe_file) == NULL) + for(;;) { + char line[BUFSIZ]; + int nbytes_r = read(pipefd[0], line, BUFSIZ); + + if(nbytes_r == 0) { + break; + } else if(nbytes_r == -1) { + if (errno != EINTR) { + _alpm_log(handle, ALPM_LOG_ERROR, _("failed to read from pipe (%s)\n"), strerror(errno)); + retval = 1; break; - alpm_logaction(handle, "%s", line); - EVENT(handle, ALPM_EVENT_SCRIPTLET_INFO, line, NULL); + } + continue; } - fclose(pipe_file); + + /* we'll need to manually add a newline if the read data doesn't end with one already */ + need_newline = (line[nbytes_r - 1] != '\n'); + + alpm_logaction(handle, "%s", line); + EVENT(handle, ALPM_EVENT_SCRIPTLET_INFO, line, NULL); + } + CLOSE(pipefd[0]); + + if (need_newline) { + alpm_logaction(handle, "\n"); } while(waitpid(pid, &status, 0) == -1) { @@ -582,11 +593,6 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) } } - /* report error from above after the child has exited */ - if(retval != 0) { - _alpm_log(handle, ALPM_LOG_ERROR, _("could not open pipe (%s)\n"), strerror(errno)); - goto cleanup; - } /* check the return status, make sure it is 0 (success) */ if(WIFEXITED(status)) { _alpm_log(handle, ALPM_LOG_DEBUG, "call to waitpid succeeded\n"); -- 1.8.0.2
Derp... ignore, send the wrong patch...
On Fri, May 10, 2013 at 10:41:41PM +1000, Allan McRae wrote:
On 09/05/13 16:48, Allan McRae wrote:
On 09/05/13 16:40, Simon Gomizelj wrote:
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root);
Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman.
I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension?
Discussed on IRC. I'd prefer to explicitly check for "." and ".." rather than have the restriction of three.
Allan
Just checking it starts with '.' should be sufficient. It will rule out '..' and the filename is already explicitly restricted from containing '/'.
On 22/05/13 14:41, Simon Gomizelj wrote:
On Fri, May 10, 2013 at 10:41:41PM +1000, Allan McRae wrote:
On 09/05/13 16:48, Allan McRae wrote:
On 09/05/13 16:40, Simon Gomizelj wrote:
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root);
Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman.
I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension?
Discussed on IRC. I'd prefer to explicitly check for "." and ".." rather than have the restriction of three.
Allan
Just checking it starts with '.' should be sufficient. It will rule out '..' and the filename is already explicitly restricted from containing '/'.
pkgname='.' works (somewhat). I guess pkgname=".foobar" is more plausible. Allan
On Wed, May 22, 2013 at 02:51:54PM +1000, Allan McRae wrote:
On 22/05/13 14:41, Simon Gomizelj wrote:
On Fri, May 10, 2013 at 10:41:41PM +1000, Allan McRae wrote:
On 09/05/13 16:48, Allan McRae wrote:
On 09/05/13 16:40, Simon Gomizelj wrote:
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root);
Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman.
I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension?
Discussed on IRC. I'd prefer to explicitly check for "." and ".." rather than have the restriction of three.
Allan
Just checking it starts with '.' should be sufficient. It will rule out '..' and the filename is already explicitly restricted from containing '/'.
pkgname='.' works (somewhat). I guess pkgname=".foobar" is more plausible.
Allan
falconindy and I has a discussion on irc about what constitutes a valid filename and I think we settled on the idea that a hidden file should be invalid. We could just move the dot check all together. So long as the filename doesn't contain a '/', its not a filepath.
On 22/05/13 16:19, Simon Gomizelj wrote:
On Wed, May 22, 2013 at 02:51:54PM +1000, Allan McRae wrote:
On 22/05/13 14:41, Simon Gomizelj wrote:
On Fri, May 10, 2013 at 10:41:41PM +1000, Allan McRae wrote:
On 09/05/13 16:48, Allan McRae wrote:
On 09/05/13 16:40, Simon Gomizelj wrote:
size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root);
Do we actually need to recalculate this each time? Maybe its worth cacheing somewhere. I'm sure there's more validation that could be done within pacman.
I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension?
Discussed on IRC. I'd prefer to explicitly check for "." and ".." rather than have the restriction of three.
Allan
Just checking it starts with '.' should be sufficient. It will rule out '..' and the filename is already explicitly restricted from containing '/'.
pkgname='.' works (somewhat). I guess pkgname=".foobar" is more plausible.
Allan
falconindy and I has a discussion on irc about what constitutes a valid filename and I think we settled on the idea that a hidden file should be invalid.
We could just move the dot check all together. So long as the filename doesn't contain a '/', its not a filepath.
We need a decision here so this patch can get pushed and we can finalise a maintenance release. I vote detecting "." and "..". and any filename containing "/". I.e. detect all paths and only paths. Allan
On 28/05/13 11:20, Allan McRae wrote:
On 22/05/13 16:19, Simon Gomizelj wrote:
On Wed, May 22, 2013 at 02:51:54PM +1000, Allan McRae wrote:
On 22/05/13 14:41, Simon Gomizelj wrote:
On Fri, May 10, 2013 at 10:41:41PM +1000, Allan McRae wrote:
On 09/05/13 16:48, Allan McRae wrote:
On 09/05/13 16:40, Simon Gomizelj wrote: > size_t cache_len = strlen(db->handle->dbpath) + strlen(db->handle->root); > > Do we actually need to recalculate this each time? Maybe its worth > cacheing somewhere. I'm sure there's more validation that could be > done within pacman. > > I'll leave the min length for now.
Why? What does three characters give you that one does not? I'm assuming an "a.Z" extension. By why do we need an extension?
Discussed on IRC. I'd prefer to explicitly check for "." and ".." rather than have the restriction of three.
Allan
Just checking it starts with '.' should be sufficient. It will rule out '..' and the filename is already explicitly restricted from containing '/'.
pkgname='.' works (somewhat). I guess pkgname=".foobar" is more plausible.
Allan
falconindy and I has a discussion on irc about what constitutes a valid filename and I think we settled on the idea that a hidden file should be invalid.
We could just move the dot check all together. So long as the filename doesn't contain a '/', its not a filepath.
We need a decision here so this patch can get pushed and we can finalise a maintenance release.
I vote detecting "." and "..". and any filename containing "/". I.e. detect all paths and only paths.
Bah - hidden files for packages can only be a bad thing... Sent a patch for makepkg to prevent packages starting with a ".". Ack -> maint for this patch.
Currently we make no effort to validate the %FILENAME% field in the repo db. This allows for relative paths to be considered valid. A carefully crafted db entry with a malicious relative path, (e.g. `../../../../etc/passwd`) will cause pacman to to overwrite _any_ file on the target's machine. Add the following validation: - doesn't start with '.' - doesn't contain a '/' - won't overflow PATH_MAX Signed-off-by: Simon Gomizelj <simongmzlj@gmail.com> --- lib/libalpm/be_sync.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 1cbe055..f9fd5d1 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -479,6 +479,33 @@ cleanup: return count; } +/* This function validates %FILENAME%. filename must be between 3 and + * PATH_MAX characters and cannot be contain a path */ +static int _alpm_validate_filename(alpm_db_t *db, const char *pkgname, + const char *filename) +{ + size_t len = strlen(filename); + + if(filename[0] == '.') { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is illegal\n"), db->treename, pkgname); + return -1; + } else if(memchr(filename, '/', len) != NULL) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is illegal\n"), db->treename, pkgname); + return -1; + } else if(len > PATH_MAX) { + errno = EINVAL; + _alpm_log(db->handle, ALPM_LOG_ERROR, _("%s database is inconsistent: filename " + "of package %s is too long\n"), db->treename, pkgname); + return -1; + } + + return 0; +} + #define READ_NEXT() do { \ if(_alpm_archive_fgets(archive, &buf) != ARCHIVE_OK) goto error; \ line = buf.line; \ @@ -558,6 +585,9 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, } } else if(strcmp(line, "%FILENAME%") == 0) { READ_AND_STORE(pkg->filename); + if(_alpm_validate_filename(db, pkg->name, pkg->filename) < 0) { + return -1; + } } else if(strcmp(line, "%DESC%") == 0) { READ_AND_STORE(pkg->desc); } else if(strcmp(line, "%GROUPS%") == 0) { -- 1.8.2.3
participants (2)
-
Allan McRae
-
Simon Gomizelj