[pacman-dev] [PATCH] Reject files larger than INT_MAX in read_sigfile.
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@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
On 05.06.2016 19:37, Tobias Stoeckmann wrote:
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;
I think you should fclose(fp); here.
}
+ MALLOC(*sig, st.st_size, return -1); + if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp);
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@stoeckmann.org> --- Thanks for pointing out the flaw Florian! --- 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,17 @@ 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) { + fclose(fp); 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
On 06/05/16 at 07:49pm, Tobias Stoeckmann wrote:
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@stoeckmann.org> --- Thanks for pointing out the flaw Florian! --- 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,17 @@ 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) {
limits.h should be included for INT_MAX. Is there not a more meaningful limit we can use for this than INT_MAX?
+ fclose(fp); return -1; }
+ MALLOC(*sig, st.st_size, return -1);
Needs fclose(fp)
+ if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp); -- 2.8.3
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@stoeckmann.org> --- I don't know about any sane limitation of signature files, so I just took INT_MAX. It's an implementation limit of pacman. --- lib/libalpm/be_package.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c9ed770..fc455e8 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -24,6 +24,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <limits.h> /* libarchive */ #include <archive.h> @@ -700,17 +701,17 @@ 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) { + fclose(fp); return -1; } + MALLOC(*sig, st.st_size, fclose(fp); return -1); + if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp); -- 2.8.3
On 07/06/16 03:59, Tobias Stoeckmann wrote:
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@stoeckmann.org> --- I don't know about any sane limitation of signature files, so I just took INT_MAX. It's an implementation limit of pacman.
In repo-add we have: if (( pgpsigsize > 16384 )); then error "$(gettext "Invalid package signature file '%s'.")" "$pkgfile.sig" So I think we should be consistent here. Also, INT_MAX is implementation dependant (although I am not sure this varies in practice...). We don't want a signature on an arch=any package to be rejected on some systems and not others. A solution to both: #define MAX_SIGFILE_SIZE 16384 Thanks, Allan
--- lib/libalpm/be_package.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c9ed770..fc455e8 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -24,6 +24,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <limits.h>
/* libarchive */ #include <archive.h> @@ -700,17 +701,17 @@ 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) { + fclose(fp); return -1; }
+ MALLOC(*sig, st.st_size, fclose(fp); return -1); + if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp);
If signature files are larger than SIZE_MAX, not enough memory could be allocated for this file. The script repo-add rejects files which are larger than 16384 bytes, therefore handle these as errors here, too. While at it, I also rearranged the code to avoid a quite harmless TOCTOU race condition between stat() and fopen(). Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> --- lib/libalpm/be_package.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c9ed770..8307d81 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -24,6 +24,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <limits.h> /* libarchive */ #include <archive.h> @@ -695,22 +696,25 @@ error: return NULL; } +/* adopted limit from repo-add */ +#define MAX_SIGFILE_SIZE 16384 + 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 > MAX_SIGFILE_SIZE) { + fclose(fp); return -1; } + MALLOC(*sig, st.st_size, fclose(fp); return -1); + if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp); -- 2.9.0
On 19/06/16 02:41, Tobias Stoeckmann wrote:
If signature files are larger than SIZE_MAX, not enough memory could be allocated for this file. The script repo-add rejects files which are larger than 16384 bytes, therefore handle these as errors here, too.
While at it, I also rearranged the code to avoid a quite harmless TOCTOU race condition between stat() and fopen().
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Pulled to my patchqueue with subject line amended. Allan
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Florian Pritz
-
Tobias Stoeckmann