* [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy
@ 2019-07-12 21:28 Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow Vitaly Chikunov
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-12 21:28 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, linux-integrity
file2bin() may return NULL, which is set to tmp, which is passed to
memcpy. Add explicit check for it. CID 229904.
---
src/evmctl.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/evmctl.c b/src/evmctl.c
index a6d07c9..39bc3d9 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -821,7 +821,15 @@ static int verify_ima(const char *file)
if (sigfile) {
void *tmp = file2bin(file, "sig", &len);
- assert(len <= sizeof(sig));
+ if (!tmp) {
+ log_err("Failed reading: %s\n", file);
+ return -1;
+ }
+ if (len > sizeof(sig)) {
+ log_err("File is too big: %s\n", file);
+ free(tmp);
+ return -1;
+ }
memcpy(sig, tmp, len);
free(tmp);
} else {
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow
2019-07-12 21:28 [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Vitaly Chikunov
@ 2019-07-12 21:28 ` Vitaly Chikunov
2019-07-15 19:08 ` Mimi Zohar
2019-07-12 21:28 ` [PATCH v1 3/5] ima-evm-utils: Fix memory leak in get_password Vitaly Chikunov
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-12 21:28 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, linux-integrity
caps_str is passed from command line but copied into fixed-size buffer.
CID 229895.
---
src/evmctl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/evmctl.c b/src/evmctl.c
index 39bc3d9..e07cff4 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -409,8 +409,9 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
} else if (!strcmp(*xattrname, XATTR_NAME_CAPS) && (hmac_flags & HMAC_FLAG_CAPS_SET)) {
if (!caps_str)
continue;
- strcpy(xattr_value, caps_str);
err = strlen(caps_str);
+ assert(err < sizeof(xattr_value));
+ strcpy(xattr_value, caps_str);
} else {
err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
if (err < 0) {
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 3/5] ima-evm-utils: Fix memory leak in get_password
2019-07-12 21:28 [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow Vitaly Chikunov
@ 2019-07-12 21:28 ` Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations Vitaly Chikunov
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-12 21:28 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, linux-integrity
Free password buffer when return NULL.
Partially fix CID 229894.
---
src/evmctl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/evmctl.c b/src/evmctl.c
index e07cff4..c556d05 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1827,6 +1827,7 @@ static char *get_password(void)
if (tcsetattr(fileno(stdin), TCSANOW, &tmp_flags) != 0) {
perror("tcsetattr");
+ free(password);
return NULL;
}
@@ -1836,6 +1837,7 @@ static char *get_password(void)
/* restore terminal */
if (tcsetattr(fileno(stdin), TCSANOW, &flags) != 0) {
perror("tcsetattr");
+ free(password);
return NULL;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations
2019-07-12 21:28 [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 3/5] ima-evm-utils: Fix memory leak in get_password Vitaly Chikunov
@ 2019-07-12 21:28 ` Vitaly Chikunov
2019-07-15 19:09 ` Mimi Zohar
2019-07-12 21:28 ` [PATCH v1 5/5] ima-evm-utils: Add more error checking in add_file_hash Vitaly Chikunov
2019-07-15 19:08 ` [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Mimi Zohar
4 siblings, 1 reply; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-12 21:28 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, linux-integrity
Check stat(2) return value, use fstat(2) to avoid race between
stat() and fopen(), remove now unused get_filesize().
Fixes CID 229889.
---
src/evmctl.c | 25 ++++++++++++++++++++-----
src/imaevm.h | 1 -
src/libimaevm.c | 8 --------
3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/src/evmctl.c b/src/evmctl.c
index c556d05..f60c12e 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -175,9 +175,10 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
static unsigned char *file2bin(const char *file, const char *ext, int *size)
{
FILE *fp;
- int len;
+ size_t len;
unsigned char *data;
char name[strlen(file) + (ext ? strlen(ext) : 0) + 2];
+ struct stat stats;
if (ext)
sprintf(name, "%s.%s", file, ext);
@@ -186,18 +187,32 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
log_info("Reading to %s\n", name);
- len = get_filesize(name);
fp = fopen(name, "r");
if (!fp) {
log_err("Failed to open: %s\n", name);
return NULL;
}
+ if (fstat(fileno(fp), &stats) == -1) {
+ log_err("Failed to fstat: %s (%s)\n", name, strerror(errno));
+ fclose(fp);
+ return NULL;
+ }
+ len = stats.st_size;
+
data = malloc(len);
- if (!fread(data, len, 1, fp))
- len = 0;
+ if (!data) {
+ log_err("Failed to malloc %zu bytes: %s\n", len, name);
+ fclose(fp);
+ return NULL;
+ }
+ if (fread(data, len, 1, fp) != len) {
+ log_err("Failed to fread %zu bytes: %s\n", len, name);
+ fclose(fp);
+ return NULL;
+ }
fclose(fp);
- *size = len;
+ *size = (int)len;
return data;
}
diff --git a/src/imaevm.h b/src/imaevm.h
index dc81a3a..36050f4 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -211,7 +211,6 @@ extern struct libevm_params params;
void do_dump(FILE *fp, const void *ptr, int len, bool cr);
void dump(const void *ptr, int len);
-int get_filesize(const char *filename);
int ima_calc_hash(const char *file, uint8_t *hash);
int get_hash_algo(const char *algo);
RSA *read_pub_key(const char *keyfile, int x509);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index f8ab812..1562aaf 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -116,14 +116,6 @@ const char *get_hash_algo_by_id(int algo)
return "unknown";
}
-int get_filesize(const char *filename)
-{
- struct stat stats;
- /* Need to know the file length */
- stat(filename, &stats);
- return (int)stats.st_size;
-}
-
static inline off_t get_fdsize(int fd)
{
struct stat stats;
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 5/5] ima-evm-utils: Add more error checking in add_file_hash
2019-07-12 21:28 [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Vitaly Chikunov
` (2 preceding siblings ...)
2019-07-12 21:28 ` [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations Vitaly Chikunov
@ 2019-07-12 21:28 ` Vitaly Chikunov
2019-07-15 19:08 ` [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Mimi Zohar
4 siblings, 0 replies; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-12 21:28 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, linux-integrity
Check return value of fstat(2) in add_file_hash() and remove
now unused get_fdsize().
---
src/libimaevm.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 1562aaf..ae487f9 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -116,20 +116,13 @@ const char *get_hash_algo_by_id(int algo)
return "unknown";
}
-static inline off_t get_fdsize(int fd)
-{
- struct stat stats;
- /* Need to know the file length */
- fstat(fd, &stats);
- return stats.st_size;
-}
-
static int add_file_hash(const char *file, EVP_MD_CTX *ctx)
{
uint8_t *data;
int err = -1, bs = DATA_SIZE;
off_t size, len;
FILE *fp;
+ struct stat stats;
fp = fopen(file, "r");
if (!fp) {
@@ -143,7 +136,12 @@ static int add_file_hash(const char *file, EVP_MD_CTX *ctx)
goto out;
}
- for (size = get_fdsize(fileno(fp)); size; size -= len) {
+ if (fstat(fileno(fp), &stats) == -1) {
+ log_err("Failed to fstat: %s (%s)\n", file, strerror(errno));
+ goto out;
+ }
+
+ for (size = stats.st_size; size; size -= len) {
len = MIN(size, bs);
if (!fread(data, len, 1, fp)) {
if (ferror(fp)) {
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy
2019-07-12 21:28 [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Vitaly Chikunov
` (3 preceding siblings ...)
2019-07-12 21:28 ` [PATCH v1 5/5] ima-evm-utils: Add more error checking in add_file_hash Vitaly Chikunov
@ 2019-07-15 19:08 ` Mimi Zohar
4 siblings, 0 replies; 10+ messages in thread
From: Mimi Zohar @ 2019-07-15 19:08 UTC (permalink / raw)
To: Vitaly Chikunov, Mimi Zohar, Dmitry Kasatkin, linux-integrity
On Sat, 2019-07-13 at 00:28 +0300, Vitaly Chikunov wrote:
> file2bin() may return NULL, which is set to tmp, which is passed to
> memcpy. Add explicit check for it. CID 229904.
Maybe move the CID to a "Fixes" tag with an indication of the CID
origin.
> ---
> src/evmctl.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/evmctl.c b/src/evmctl.c
> index a6d07c9..39bc3d9 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -821,7 +821,15 @@ static int verify_ima(const char *file)
> if (sigfile) {
> void *tmp = file2bin(file, "sig", &len);
>
> - assert(len <= sizeof(sig));
Thanks for removing the "assert". It would stop the measurement list
verification or walking a file system in the middle.
> + if (!tmp) {
> + log_err("Failed reading: %s\n", file);
> + return -1;
> + }
> + if (len > sizeof(sig)) {
> + log_err("File is too big: %s\n", file);
We're reading the file signature file. Perhaps say,"File signature is
...".
> + free(tmp);
> + return -1;
> + }
> memcpy(sig, tmp, len);
> free(tmp);
> } else {
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow
2019-07-12 21:28 ` [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow Vitaly Chikunov
@ 2019-07-15 19:08 ` Mimi Zohar
2019-07-15 20:05 ` Vitaly Chikunov
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-07-15 19:08 UTC (permalink / raw)
To: Vitaly Chikunov, Dmitry Kasatkin, linux-integrity
On Sat, 2019-07-13 at 00:28 +0300, Vitaly Chikunov wrote:
> caps_str is passed from command line but copied into fixed-size buffer.
> CID 229895.
> ---
> src/evmctl.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 39bc3d9..e07cff4 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -409,8 +409,9 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> } else if (!strcmp(*xattrname, XATTR_NAME_CAPS) && (hmac_flags & HMAC_FLAG_CAPS_SET)) {
> if (!caps_str)
> continue;
> - strcpy(xattr_value, caps_str);
> err = strlen(caps_str);
> + assert(err < sizeof(xattr_value));
"calc_evm_hash" can be called while walking and labeling, or
verifying, a file system. We probably don't want to abruptly end it.
Maybe emit an error message and return and error?
> + strcpy(xattr_value, caps_str);
> } else {
> err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
> if (err < 0) {
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations
2019-07-12 21:28 ` [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations Vitaly Chikunov
@ 2019-07-15 19:09 ` Mimi Zohar
2019-07-15 20:04 ` Vitaly Chikunov
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-07-15 19:09 UTC (permalink / raw)
To: Vitaly Chikunov, Mimi Zohar, Dmitry Kasatkin, linux-integrity
On Sat, 2019-07-13 at 00:28 +0300, Vitaly Chikunov wrote:
<snip>
> @@ -186,18 +187,32 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
>
> log_info("Reading to %s\n", name);
>
> - len = get_filesize(name);
> fp = fopen(name, "r");
> if (!fp) {
> log_err("Failed to open: %s\n", name);
> return NULL;
> }
> + if (fstat(fileno(fp), &stats) == -1) {
> + log_err("Failed to fstat: %s (%s)\n", name, strerror(errno));
> + fclose(fp);
> + return NULL;
> + }
> + len = stats.st_size;
> +
> data = malloc(len);
> - if (!fread(data, len, 1, fp))
> - len = 0;
> + if (!data) {
> + log_err("Failed to malloc %zu bytes: %s\n", len, name);
Missing "free(data)"
> + fclose(fp);
> + return NULL;
> + }
> + if (fread(data, len, 1, fp) != len) {
> + log_err("Failed to fread %zu bytes: %s\n", len, name);
> + fclose(fp);
and here
> + return NULL;
> + }
> fclose(fp);
>
> - *size = len;
> + *size = (int)len;
> return data;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations
2019-07-15 19:09 ` Mimi Zohar
@ 2019-07-15 20:04 ` Vitaly Chikunov
0 siblings, 0 replies; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-15 20:04 UTC (permalink / raw)
To: Mimi Zohar; +Cc: Mimi Zohar, Dmitry Kasatkin, linux-integrity
Mimi,
On Mon, Jul 15, 2019 at 03:09:34PM -0400, Mimi Zohar wrote:
> On Sat, 2019-07-13 at 00:28 +0300, Vitaly Chikunov wrote:
> > @@ -186,18 +187,32 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
> >
> > log_info("Reading to %s\n", name);
> >
> > - len = get_filesize(name);
> > fp = fopen(name, "r");
> > if (!fp) {
> > log_err("Failed to open: %s\n", name);
> > return NULL;
> > }
> > + if (fstat(fileno(fp), &stats) == -1) {
> > + log_err("Failed to fstat: %s (%s)\n", name, strerror(errno));
> > + fclose(fp);
> > + return NULL;
> > + }
> > + len = stats.st_size;
> > +
> > data = malloc(len);
> > - if (!fread(data, len, 1, fp))
> > - len = 0;
> > + if (!data) {
> > + log_err("Failed to malloc %zu bytes: %s\n", len, name);
>
> Missing "free(data)"
In the next patch set I will apply all your suggestions except this one,
because this is return where data was not allocated.
> > + fclose(fp);
> > + return NULL;
> > + }
> > + if (fread(data, len, 1, fp) != len) {
> > + log_err("Failed to fread %zu bytes: %s\n", len, name);
> > + fclose(fp);
>
> and here
> > + return NULL;
> > + }
> > fclose(fp);
> >
> > - *size = len;
> > + *size = (int)len;
> > return data;
> > }
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow
2019-07-15 19:08 ` Mimi Zohar
@ 2019-07-15 20:05 ` Vitaly Chikunov
0 siblings, 0 replies; 10+ messages in thread
From: Vitaly Chikunov @ 2019-07-15 20:05 UTC (permalink / raw)
To: Mimi Zohar; +Cc: Dmitry Kasatkin, linux-integrity
On Mon, Jul 15, 2019 at 03:08:55PM -0400, Mimi Zohar wrote:
> On Sat, 2019-07-13 at 00:28 +0300, Vitaly Chikunov wrote:
> > caps_str is passed from command line but copied into fixed-size buffer.
> > CID 229895.
> > ---
> > src/evmctl.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 39bc3d9..e07cff4 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -409,8 +409,9 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > } else if (!strcmp(*xattrname, XATTR_NAME_CAPS) && (hmac_flags & HMAC_FLAG_CAPS_SET)) {
> > if (!caps_str)
> > continue;
> > - strcpy(xattr_value, caps_str);
> > err = strlen(caps_str);
> > + assert(err < sizeof(xattr_value));
>
> "calc_evm_hash" can be called while walking and labeling, or
> verifying, a file system. We probably don't want to abruptly end it.
> Maybe emit an error message and return and error?
Ok. I will also add similar checks for selinux_str and ima_str.
Thanks,
>
>
> > + strcpy(xattr_value, caps_str);
> > } else {
> > err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
> > if (err < 0) {
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-15 20:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-12 21:28 [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 2/5] ima-evm-utils: Fix possible strcpy overflow Vitaly Chikunov
2019-07-15 19:08 ` Mimi Zohar
2019-07-15 20:05 ` Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 3/5] ima-evm-utils: Fix memory leak in get_password Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 4/5] ima-evm-utils: Fix file2bin stat and fopen relations Vitaly Chikunov
2019-07-15 19:09 ` Mimi Zohar
2019-07-15 20:04 ` Vitaly Chikunov
2019-07-12 21:28 ` [PATCH v1 5/5] ima-evm-utils: Add more error checking in add_file_hash Vitaly Chikunov
2019-07-15 19:08 ` [PATCH v1 1/5] ima-evm-utils: Fix null dereference from file2bin to memcpy Mimi Zohar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).