vhost: fix insecure temporary file

PATCH In review Jin Yu 2 participants 4 messages Nov 26, 2019 – Nov 27, 2019

Share & tools

Jin Yu

author

2019-11-26 07:36 UTC #0 lore raw

PATCH 1/1 vhost: fix insecure temporary file

When using mkstemp(), remember to safely set the umask before to restrict the resulting temporary file permissions to only the owner.

Coverity issue: 350367

Fixes d87f1a1cb7b6 ("vhost: support inflight info sharing")

Cc [email protected]

Signed-off-by Jin Yu <[email protected]>

---
 lib/librte_vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)

lib/librte_vhost/vhost_user.c +2 -0

index 0cfb8b792..1a68e23e3 100644
@@ -1342,6 +1342,7 @@ inflight_mem_alloc(const char *name, size_t size, int *fd)
 	RTE_SET_USED(name);
 #endif
 	if (mfd == -1) {
+		mode_t mask = umask(0600);
 		mfd = mkstemp(fname);
 		if (mfd == -1) {
 			RTE_LOG(ERR, VHOST_CONFIG,

@@ -1349,6 +1350,7 @@ inflight_mem_alloc(const char *name, size_t size, int *fd)
 			return NULL;
 		}
 
+		umask(mask);
 		unlink(fname);
 	}

Signature · 1 line

2.17.2

Bruce Richardson

2019-11-26 10:00 UTC #1 lore raw

Quoted 18 lines · Jin Yu

When using mkstemp(), remember to safely set the umask before to restrict the resulting temporary file permissions to only the owner.
Coverity issue: 350367
Fixes d87f1a1cb7b6 ("vhost: support inflight info sharing")
Cc [email protected]
Signed-off-by Jin Yu <[email protected]>
---
 lib/librte_vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)
lib/librte_vhost/vhost_user.c +0 -0
index 0cfb8b792..1a68e23e3 100644

lib/librte_vhost/vhost_user.c inflight_mem_alloc(const char *name, size_t size, int *fd) · line 1346

Quoted 5 lines

lib/librte_vhost/vhost_user.c +1 -0

@@ -1342,6 +1342,7 @@ inflight_mem_alloc(const char *name, size_t size, int *fd)
 	RTE_SET_USED(name);
 #endif
 	if (mfd == -1) {
+		mode_t mask = umask(0600);
 		mfd = mkstemp(fname);

Setting the umask is unnecessary, as if you read the man page for mkstemp:

"The file is created with permissions 0600, that is, read plus write for owner only."

I am aware that coverity flags this as a potential issue, but if you follow the link from the coverity issue to CWE-377 on cwe.mitre.org, you can find the following at the end of the "Notes" section:

"Finally, mkstemp() is a reasonably safe way create temporary files. It will attempt to create and open a unique file based on a filename template provided by the user combined with a series of randomly generated characters. If it is unable to create such a file, it will fail and return -1. On modern systems the file is opened using mode 0600, which means the file will be secure from tampering unless the user explicitly changes its access permissions. However, mkstemp() still suffers from the use of predictable file names and can leave an application vulnerable to denial of service attacks if an attacker causes mkstemp() to fail by predicting and pre-creating the filenames to be used."

So it seems that for creating temporary files, mkstemp() is probably the best function we can use. Therefore, I recommend not trying to patch this issue and just mark the issue as "ignore" in coverity.

Regards,
/Bruce

Yu, Jin

author

2019-11-27 02:19 UTC #2 lore raw

Quoted 56 lines

-----Original Message-----
From: Bruce Richardson <[email protected]>
Sent: Tuesday, November 26, 2019 6:00 PM
To: Yu, Jin <[email protected]>
Cc: Maxime Coquelin <[email protected]>; Bie, Tiwei <[email protected]>; Wang, Zhihong <[email protected]>; [email protected]; [email protected]
Subject: Re: [dpdk-dev] [PATCH] vhost: fix insecure temporary file
lib/librte_vhost/vhost_user.c inflight_mem_alloc(const char *name, size_t size,
Quoted 17 lines · Jin Yu
When using mkstemp(), remember to safely set the umask before to restrict the resulting temporary file permissions to only the owner.
Coverity issue: 350367
Fixes d87f1a1cb7b6 ("vhost: support inflight info sharing")
Cc [email protected]
Signed-off-by Jin Yu <[email protected]>
---
 lib/librte_vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/lib/librte_vhost/vhost_user.c index 0cfb8b792..1a68e23e3 100644
lib/librte_vhost/vhost_user.c +0 -0
@@ -1342,6 +1342,7 @@ inflight_mem_alloc(const char *name, size_t size,
int *fd)
lib/librte_vhost/vhost_user.c inflight_mem_alloc(const char *name, size_t size, int *fd) · line 1346
Quoted 5 lines
lib/librte_vhost/vhost_user.c +1 -0
 	RTE_SET_USED(name);
 #endif
 	if (mfd == -1) {
+		mode_t mask = umask(0600);
 		mfd = mkstemp(fname);
Setting the umask is unnecessary, as if you read the man page for mkstemp:
"The file is created with permissions 0600, that is, read plus write for owner only."
I am aware that coverity flags this as a potential issue, but if you follow the link from the coverity issue to CWE-377 on cwe.mitre.org, you can find the following at the end of the "Notes" section:
"Finally, mkstemp() is a reasonably safe way create temporary files. It will attempt to create and open a unique file based on a filename template provided by the user combined with a series of randomly generated characters. If it is unable to create such a file, it will fail and return -1. On modern systems the file is opened using mode 0600, which means the file will be secure from tampering unless the user explicitly changes its access permissions. However, mkstemp() still suffers from the use of predictable file names and can leave an application vulnerable to denial of service attacks if an attacker causes mkstemp() to fail by predicting and pre-creating the filenames to be used."

Quoted 3 lines

So it seems that for creating temporary files, mkstemp() is probably the best function we can use. Therefore, I recommend not trying to patch this issue and just mark the issue as "ignore" in coverity.

Yes. I agree with you. I just thought we must fix the coverity issue. So I add the umask. I would prefer to mark this issue as "ignore" in coverity.

Quoted 3 lines

Regards,
/Bruce

Bruce Richardson

2019-11-27 09:32 UTC #3 lore raw

Quoted 56 lines · Jin

Quoted 56 lines
-----Original Message-----
From: Bruce Richardson <[email protected]>
Sent: Tuesday, November 26, 2019 6:00 PM
To: Yu, Jin <[email protected]>
Cc: Maxime Coquelin <[email protected]>; Bie, Tiwei <[email protected]>; Wang, Zhihong <[email protected]>; [email protected]; [email protected]
Subject: Re: [dpdk-dev] [PATCH] vhost: fix insecure temporary file
lib/librte_vhost/vhost_user.c inflight_mem_alloc(const char *name, size_t size,
Quoted 17 lines · Jin Yu
When using mkstemp(), remember to safely set the umask before to restrict the resulting temporary file permissions to only the owner.
Coverity issue: 350367
Fixes d87f1a1cb7b6 ("vhost: support inflight info sharing")
Cc [email protected]
Signed-off-by Jin Yu <[email protected]>
---
 lib/librte_vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/lib/librte_vhost/vhost_user.c index 0cfb8b792..1a68e23e3 100644
lib/librte_vhost/vhost_user.c +0 -0
@@ -1342,6 +1342,7 @@ inflight_mem_alloc(const char *name, size_t size,
int *fd)
lib/librte_vhost/vhost_user.c inflight_mem_alloc(const char *name, size_t size, int *fd) · line 1346
Quoted 5 lines
lib/librte_vhost/vhost_user.c +1 -0
 	RTE_SET_USED(name);
 #endif
 	if (mfd == -1) {
+		mode_t mask = umask(0600);
 		mfd = mkstemp(fname);
Setting the umask is unnecessary, as if you read the man page for mkstemp:
"The file is created with permissions 0600, that is, read plus write for owner only."
I am aware that coverity flags this as a potential issue, but if you follow the link from the coverity issue to CWE-377 on cwe.mitre.org, you can find the following at the end of the "Notes" section:
"Finally, mkstemp() is a reasonably safe way create temporary files. It will attempt to create and open a unique file based on a filename template provided by the user combined with a series of randomly generated characters. If it is unable to create such a file, it will fail and return -1. On modern systems the file is opened using mode 0600, which means the file will be secure from tampering unless the user explicitly changes its access permissions. However, mkstemp() still suffers from the use of predictable file names and can leave an application vulnerable to denial of service attacks if an attacker causes mkstemp() to fail by predicting and pre-creating the filenames to be used."

Quoted 7 lines

Quoted 3 lines
So it seems that for creating temporary files, mkstemp() is probably the best function we can use. Therefore, I recommend not trying to patch this issue and just mark the issue as "ignore" in coverity.
Yes. I agree with you. I just thought we must fix the coverity issue. So I add the umask. I would prefer to mark this issue as "ignore" in coverity.
Quoted 1 line

You can try and find a better fix if you like, but setting the umask is pointless for using mkstemp. Therefore, unless you have an alternative, I recommend just marking the issue as "false positive" and set it to "ignore".

Regards,
/Bruce