- Pacman-dev - lists.archlinux.org

[pacman-dev] [PATCH] Make DUFLAGS be overrideable during configure
by Alastair Hughes 23 Jun '16

23 Jun '16

Signed-off-by: Alastair Hughes <hobbitalastair(a)gmail.com> --- configure.ac | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

1 0

[pacman-dev] [PATCH] Prevent stack overflow on symbolic link access.
by Tobias Stoeckmann 23 Jun '16

23 Jun '16

It is possible (but unlikely) to encounter very long symbolic links, which would take a few mega bytes in size. In general, such long symbolic links are impossible to create because the kernel will only accept up to PATH_MAX in length. BUT lstat() on a maliciously modified symbolic link can return a much larger size than that. In function check_file_link (src/pacman/check.c), the length of a symbolic link is used in a variable length array: char link[length]; // length being st->st_size + 1 This doesn't just allow a very theoretical overflow on 32 bit systems with 64 bit off_t (which is st_size's type), it can also overflow available stack space, leading to a segmentation fault. Symbolic links pointing to something that is longer than PATH_MAX can be considered illegal, so just get a fixed size array and reject symbolic links which are longer than that. --- src/pacman/check.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/pacman/check.c b/src/pacman/check.c index d282cc2..6e8cda7 100644 --- a/src/pacman/check.c +++ b/src/pacman/check.c @@ -132,15 +132,16 @@ static int check_file_time(const char *pkgname, const char *filepath, static int check_file_link(const char *pkgname, const char *filepath, struct stat *st, struct archive_entry *entry) { - size_t length = st->st_size + 1; - char link[length]; + ssize_t len; + char link[PATH_MAX]; - if(readlink(filepath, link, length) != st->st_size) { + len = readlink(filepath, link, sizeof(link)); + if (len == -1 || len >= PATH_MAX) { /* this should not happen */ pm_printf(ALPM_LOG_ERROR, _("unable to read symlink contents: %s\n"), filepath); return 1; } - link[length - 1] = '\0'; + link[len] = '\0'; if(strcmp(link, archive_entry_symlink(entry)) != 0) { if(!config->quiet) { -- 2.8.3

4 22

[pacman-dev] [PATCH] Reject files larger than INT_MAX in read_sigfile.
by Tobias Stoeckmann 23 Jun '16

23 Jun '16

Signature files larger than INT_MAX are already suspicious, but if they are larger than SIZE_MAX, this code couldn't even copy them into memory, accepting them as "blank" files at worst. While adding the INT_MAX check, I also rearranged the code to avoid a quite harmless TOCTOU race condition between stat() and fopen(). Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- lib/libalpm/be_package.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c9ed770..055fb1e 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -700,17 +700,16 @@ static int read_sigfile(const char *sigpath, unsigned char **sig) struct stat st; FILE *fp; - if(stat(sigpath, &st) != 0) { + if((fp = fopen(sigpath, "rb")) == NULL) { return -1; } - MALLOC(*sig, st.st_size, return -1); - - if((fp = fopen(sigpath, "rb")) == NULL) { - free(*sig); + if(fstat(fileno(fp), &st) != 0 || st.st_size > INT_MAX) { return -1; } + MALLOC(*sig, st.st_size, return -1); + if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp); -- 2.8.3

4 7

[pacman-dev] [PATCH] Fix out of boundary reads in pacsort.
by Tobias Stoeckmann 18 Jun '16

18 Jun '16

This patch fixes two out of bound reads in pacsort (as they are of the same error type, I didn't see a reason to split it into two commits). The first one is triggered by a very large "key column" and the delimiter '\0'. The return value of strchr() is not checked for the special case in which it reaches the end of the version string. I recommend to simply not allow the '\0' separator, because file names cannot contain it. And therefore, it cannot be a sane column separator in file names for versions either. You can trigger it by running this command: $ echo -e "-.pkg.tar.xz\n-.pkg.tar.xz" | pacsort -k 999999 -t '\0' Segmentation fault The second one is triggered due to a wrong assumption in fnmatch pattern. It is assumed that "*-*.pkg.tar.?z" will only match file names which contain a dash. But in fact, it also accepts files without one, if at least one of the directories contains it. One example would be "/my-path/file.pkg.tar.xz". While parsing the version string, the return value of strrchr() looking for the dash is not checked anymore. In this case, it would return NULL and further pointer arithmetic on it turns it into SIZE_MAX. Eventually, pacman segfaults while traversing this memory area for needed chars: $ echo "-/.pkg.tar.xz" | pacsort Segmentation fault Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- src/util/pacsort.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/pacsort.c b/src/util/pacsort.c index 662b250..65f67bd 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -228,6 +228,9 @@ static struct input_t *input_new(const char *path, int pathlen) } pkgver_end = strrchr(in->pkgname, '-'); + if(pkgver_end == NULL || pkgver_end == in->pkgname) { + return in; + } /* read backwards through pkgrel */ for(in->pkgver = pkgver_end - 1; @@ -410,8 +413,6 @@ static char escape_char(const char *string) return '\n'; case 'v': return '\v'; - case '0': - return '\0'; default: return INVALD_ESCAPE_CHAR; } -- 2.8.3

5 8

[pacman-dev] [PATCH] Release resources on error paths.
by Tobias Stoeckmann 13 Jun '16

13 Jun '16

Some resources (memory or file descriptors) are not released on all error paths. Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- Yes it's rather ironic to send this patch after forgetting one on my own just now. ;) --- lib/libalpm/add.c | 5 ++++- lib/libalpm/backup.c | 5 +++-- lib/libalpm/be_local.c | 18 +++++++++++++++--- lib/libalpm/be_package.c | 1 + 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index f5c9a95..d132e52 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -466,7 +466,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, } } - /* prepare directory for database entries so permission are correct after + /* prepare directory for database entries so permissions are correct after changelog/install script installation */ if(_alpm_local_db_prepare(db, newpkg)) { alpm_logaction(handle, ALPM_CALLER_PREFIX, @@ -503,6 +503,9 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, _alpm_log(handle, ALPM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); _alpm_archive_read_free(archive); + if(cwdfd >= 0) { + close(cwdfd); + } close(fd); ret = -1; goto cleanup; diff --git a/lib/libalpm/backup.c b/lib/libalpm/backup.c index f622589..50bad5e 100644 --- a/lib/libalpm/backup.c +++ b/lib/libalpm/backup.c @@ -48,9 +48,10 @@ int _alpm_split_backup(const char *string, alpm_backup_t **backup) ptr++; /* now str points to the filename and ptr points to the hash */ STRDUP((*backup)->name, str, FREE(str); return -1); - STRDUP((*backup)->hash, ptr, FREE(str); return -1); + STRDUP((*backup)->hash, ptr, FREE((*backup)->name); FREE(str); return -1); FREE(str); - return 0;} + return 0; +} /* Look for a filename in a alpm_pkg_t.backup list. If we find it, * then we return the full backup entry. diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index f817822..0b351f9 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -794,7 +794,7 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) _alpm_strip_newline(line, 0); if(strcmp(line, "%FILES%") == 0) { size_t files_count = 0, files_size = 0, len; - alpm_file_t *files = NULL; + alpm_file_t *files = NULL, *newfiles; while(safe_fgets(line, sizeof(line), fp) && (len = _alpm_strip_newline(line, 0))) { @@ -805,13 +805,18 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) /* since we know the length of the file string already, * we can do malloc + memcpy rather than strdup */ len += 1; - MALLOC(files[files_count].name, len, goto error); + MALLOC(files[files_count].name, len, goto nomem); memcpy(files[files_count].name, line, len); files_count++; } /* attempt to hand back any memory we don't need */ if(files_count > 0) { - files = realloc(files, sizeof(alpm_file_t) * files_count); + newfiles = realloc(files, sizeof(alpm_file_t) * files_count); + if(newfiles == NULL) { + goto nomem; + } + files = newfiles; + /* make sure the list is sorted */ qsort(files, files_count, sizeof(alpm_file_t), _alpm_files_cmp); } else { @@ -819,6 +824,13 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) } info->files.count = files_count; info->files.files = files; + continue; +nomem: + while(files_count > 0) { + FREE(files[--files_count].name); + } + FREE(files); + goto error; } else if(strcmp(line, "%BACKUP%") == 0) { while(safe_fgets(line, sizeof(line), fp) && _alpm_strip_newline(line, 0)) { alpm_backup_t *backup; diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c9ed770..a79c0c5 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -762,6 +762,7 @@ int SYMEXPORT alpm_pkg_load(alpm_handle_t *handle, const char *filename, int ful if(fail) { _alpm_log(handle, ALPM_LOG_ERROR, _("required key missing from keyring\n")); + free(sigpath); return -1; } } -- 2.8.3

3 3

[pacman-dev] [PATCH] pacman-key: reject armored signatures
by Allan McRae 13 Jun '16

13 Jun '16

pacman expectes and unarmored signature. makepkg forces the generation of unarmored signatures, and repo-add will reject any armored signature. For consistency pacman-key should also reject armored signatures. Signed-off-by: Allan McRae <allan(a)archlinux.org> --- scripts/pacman-key.sh.in | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 0db0952..ffefc68 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -483,6 +483,10 @@ verify_sig() { local ret=0 for sig; do msg "Checking %s..." "$sig" + if grep -q 'BEGIN PGP SIGNATURE' "$sig"; then + error "$(gettext "Cannot use armored signatures for packages: %s")" "$sig" + return 1 + fi if ! "${GPG_PACMAN[@]}" --status-fd 1 --verify "$sig" | grep -qE '^\[GNUPG:\] TRUST_(FULLY|ULTIMATE).*$'; then error "$(gettext "The signature identified by %s could not be verified.")" "$sig" ret=1 -- 2.8.3

2 2

[pacman-dev] [PATCH] Files that contain references to srcdir and pkgdir are listed
by Ashley Whetter 12 Jun '16

12 Jun '16

Fixes FS#31558 Signed-off-by: Ashley Whetter <ashley(a)awhetter.co.uk> --- .../libmakepkg/lint_package/build_references.sh.in | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/scripts/libmakepkg/lint_package/build_references.sh.in b/scripts/libmakepkg/lint_package/build_references.sh.in index 67c14e6..dd6b88c 100644 --- a/scripts/libmakepkg/lint_package/build_references.sh.in +++ b/scripts/libmakepkg/lint_package/build_references.sh.in @@ -29,10 +29,24 @@ source "$LIBRARY/util/message.sh" lint_package_functions+=('warn_build_references') warn_build_references() { - if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${srcdir}" ; then - warning "$(gettext "Package contains reference to %s")" "\$srcdir" + warn_single_build_references "$srcdir" srcdir + warn_single_build_references "$pkgdirbase" pkgdir +} + +warn_single_build_references() { + local to_find=$1 ref_name=$2 + + mapfile -t refs < <(cd "$pkgdir" && find_files_that_ref "$to_find") + if (( ${#refs[*]} )); then + warning "$(gettext "Package contains reference to %s")" "\$$ref_name" + printf " %s\n" "${refs[@]}" fi - if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${pkgdirbase}" ; then - warning "$(gettext "Package contains reference to %s")" "\$pkgdir" +} + +find_files_that_ref() { + if [[ -z "$1" ]]; then + return 1 fi + + find -type f -exec grep -Il "$1" {} + } -- 2.8.2

2 1

[pacman-dev] [PATCH] Can place local source files in directories
by Ashley Whetter 12 Jun '16

12 Jun '16

For example a source file 'mydir/myfile.sh' is now valid, whereas before 'myfile.sh' would have needed to be placed in the same directory as the PKGBUILD. Fixes FS#43020 Signed-off-by: Ashley Whetter <ashley(a)awhetter.co.uk> --- I've tested this by generating checksums, making a package, and making a source package, all with downloadable and local files. Local files were tested in both the PKGBUILD directory and a subdirectory. This doesn't work for files that have been placed under a fake protocol to force makepkg not to include the file in the source package (for example 'file://a_sub_dir/my_huge_source_file.tar.gz'). scripts/libmakepkg/source/file.sh.in | 4 +++- scripts/libmakepkg/source/local.sh.in | 2 +- scripts/libmakepkg/util/source.sh.in | 17 +++++++++++++++++ scripts/makepkg.sh.in | 8 ++++++-- 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/scripts/libmakepkg/source/file.sh.in b/scripts/libmakepkg/source/file.sh.in index 20493a6..6f17884 100644 --- a/scripts/libmakepkg/source/file.sh.in +++ b/scripts/libmakepkg/source/file.sh.in @@ -87,7 +87,9 @@ extract_file() { local filepath=$(get_filepath "$file") rm -f "$srcdir/${file}" - ln -s "$filepath" "$srcdir/" + local intermediatedirs=$(get_intermediate_dirs "$file") + mkdir -p "$srcdir/$intermediatedirs" + ln -s "$filepath" "$srcdir/$intermediatedirs" if in_array "$file" "${noextract[@]}"; then # skip source files in the noextract=() array diff --git a/scripts/libmakepkg/source/local.sh.in b/scripts/libmakepkg/source/local.sh.in index 6a4b882..ef4829a 100644 --- a/scripts/libmakepkg/source/local.sh.in +++ b/scripts/libmakepkg/source/local.sh.in @@ -33,7 +33,7 @@ download_local() { local filepath=$(get_filepath "$netfile") if [[ -n "$filepath" ]]; then - msg2 "$(gettext "Found %s")" "${filepath##*/}" + msg2 "$(gettext "Found %s")" "$netfile" else local filename=$(get_filename "$netfile") error "$(gettext "%s was not found in the build directory and is not a URL.")" "$filename" diff --git a/scripts/libmakepkg/util/source.sh.in b/scripts/libmakepkg/util/source.sh.in index 9d4ba4a..f16d3c6 100644 --- a/scripts/libmakepkg/util/source.sh.in +++ b/scripts/libmakepkg/util/source.sh.in @@ -74,6 +74,10 @@ get_filename() { filename=${filename%%.git*} fi ;; + local) + # If it is a local file then leave the full path as is + filename=$netfile + ;; *) # if it is just an URL, we only keep the last component filename="${netfile##*/}" @@ -142,3 +146,16 @@ get_downloadclient() { printf "%s\n" "$agent" } + +# Get the directories that exist before a source file +get_intermediate_dirs() { + local netfile=$1 + local intermediatedirs + + if [[ "$(get_protocol "$netfile")" = 'local' ]] && [[ "$netfile" = */* ]] + then + intermediatedirs=${netfile%/*} + fi + + printf "%s\n" "$intermediatedirs" +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f80e37a..392b1b3 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1231,8 +1231,12 @@ create_srcpackage() { if [[ "$file" = "$(get_filename "$file")" ]] || (( SOURCEONLY == 2 )); then local absfile absfile=$(get_filepath "$file") || missing_source_file "$file" - msg2 "$(gettext "Adding %s...")" "${absfile##*/}" - ln -s "$absfile" "$srclinks/$pkgbase" + msg2 "$(gettext "Adding %s...")" "$(get_filename "$file")" + + local intermediatedirs="$(get_intermediate_dirs "$file")" + mkdir -p "$srclinks/$pkgbase/$intermediatedirs" + + ln -s "$absfile" "$srclinks/$pkgbase/$intermediatedirs" fi done -- 2.8.0

2 1

[pacman-dev] [PATCH] callback.c: only print download status message once with --noprogressbar
by Ivy Foster 09 Jun '16

09 Jun '16

Syncing repos and downloading packages with --noprogressbar specified often prints the status message "downloading $foo..." to several lines in a row. --- src/pacman/callback.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index ab3e14f..468d657 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -680,8 +680,10 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) if(config->noprogressbar || cols == 0 || file_total == -1) { if(file_xfered == 0) { - printf(_("downloading %s...\n"), filename); + printf(_("downloading %s...\r"), filename); fflush(stdout); + } else if(file_xfered == file_total) { + putchar('\n'); } return; } -- 2.8.3

2 2

[pacman-dev] [PATCH] Fix OOB read and endless loop in signature parser.
by Tobias Stoeckmann 06 Jun '16

06 Jun '16

The parser in alpm_extract_keyid does not properly check boundaries, which makes it vulnerable to malicious input for out ouf bondary reads. I did not attach a proof of concept for this one, because it is very system-specific if you see a segmentation fault or not. If in doubt, you won't notice it. ;) It is also possible to trigger an endless loop with a malicious signature file on 32 bit systems. A proof of concept for this can be triggered through this crafted file (SigLevel must be properly set for local files): $ uname -m i686 $ PKG=package-1.0.tar.xz $ touch $PKG $ echo "iQEcBAABCAAGBQJXTxJiAAr/////+wA=" | base64 -d - > $PKG.sig $ sudo pacman -U $PKG _ This proof of concept uses the ability to overflow position increments. By setting lengths to specific values ((size_t)-5 in this example), the position incrementation effectively ends up at its old position. This patch does not use GCC-specific overflow checking routines, because I am not sure if that's applicable for pacman's design goals. Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- Feedback is very appreciated. :) --- lib/libalpm/signing.c | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 0267158..7dc7694 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -982,6 +982,21 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist) return 0; } +#define LENGTH_CHECK(l, p, a) \ + do { \ + if ((a) == 0 || (l) - (p) <= (a)) { \ + _alpm_log(handle, ALPM_LOG_ERROR, \ + _("%s: signature format error"), identifier); \ + return -1; \ + } \ + } while(0) + +#define SAFE_ADD(l, p, a) \ + do { \ + LENGTH_CHECK((l), p, (a)); \ + p = p + (a); \ + } while(0) + /** * Extract the Issuer Key ID from a signature * @param sig PGP signature @@ -1018,18 +1033,21 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, switch(sig[pos] & 0x03) { case 0: + LENGTH_CHECK(len, pos, 1); blen = sig[pos + 1]; - pos = pos + 2; + SAFE_ADD(len, pos, 2); break; case 1: + LENGTH_CHECK(len, pos, 2); blen = (sig[pos + 1] << 8) | sig[pos + 2]; - pos = pos + 3; + SAFE_ADD(len, pos, 3); break; case 2: + LENGTH_CHECK(len, pos, 4); blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4]; - pos = pos + 5; + SAFE_ADD(len, pos, 5); break; case 3: @@ -1046,6 +1064,7 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, return -1; } + LENGTH_CHECK(len, pos, 1); if(sig[pos + 1] != 0x00) { /* not a signature of a binary document */ _alpm_log(handle, ALPM_LOG_ERROR, @@ -1053,32 +1072,38 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, return -1; } - pos = pos + 4; + SAFE_ADD(len, pos, 4); + LENGTH_CHECK(len, pos, 1); hlen = (sig[pos] << 8) | sig[pos + 1]; - pos = pos + hlen + 2; + SAFE_ADD(len, pos, hlen + 2); + LENGTH_CHECK(len, pos, 1); ulen = (sig[pos] << 8) | sig[pos + 1]; - pos = pos + 2; + SAFE_ADD(len, pos, 2); spos = pos; + LENGTH_CHECK(len, pos, ulen); while(spos < pos + ulen) { if(sig[spos] < 192) { slen = sig[spos]; spos = spos + 1; } else if(sig[spos] < 255) { + LENGTH_CHECK(pos + ulen, spos, 1); slen = (sig[spos] << 8) | sig[spos + 1]; - spos = spos + 2; + SAFE_ADD(pos + ulen, spos, 2); } else { + LENGTH_CHECK(pos + ulen, spos, 4); slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4]; - spos = spos + 5; + SAFE_ADD(pos + ulen, spos, 5); } if(sig[spos] == 16) { /* issuer key ID */ char key[17]; size_t i; + LENGTH_CHECK(pos + ulen, spos, 8); for (i = 0; i < 8; i++) { sprintf(&key[i * 2], "%02X", sig[spos + i + 1]); } @@ -1086,10 +1111,11 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, break; } - spos = spos + slen; + SAFE_ADD(pos + ulen + 1, spos, slen); } - pos = pos + (blen - hlen - 8); + LENGTH_CHECK(blen, hlen, 8); + SAFE_ADD(len + 1, pos, blen - hlen - 8); } return 0; -- 2.8.3

1 0