diff mbox series

vhost: fix insecure temporary file

Message ID 20191126151900.70915-1-jin.yu@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix insecure temporary file |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-compilation fail Compilie Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Jin Yu Nov. 26, 2019, 3:19 p.m. UTC 
  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: stable@dpdk.org

Signed-off-by: Jin Yu <jin.yu@intel.com>
---
 lib/librte_vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Bruce Richardson Nov. 26, 2019, 10 a.m. UTC | #1 
On Tue, Nov 26, 2019 at 11:19:00PM +0800, Jin Yu wrote:
> 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: stable@dpdk.org
> 
> Signed-off-by: Jin Yu <jin.yu@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 0cfb8b792..1a68e23e3 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -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
Jin Yu Nov. 27, 2019, 2:19 a.m. UTC | #2 
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, November 26, 2019 6:00 PM
> To: Yu, Jin <jin.yu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: fix insecure temporary file
> 
> On Tue, Nov 26, 2019 at 11:19:00PM +0800, Jin Yu wrote:
> > 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: stable@dpdk.org
> >
> > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/librte_vhost/vhost_user.c
> > b/lib/librte_vhost/vhost_user.c index 0cfb8b792..1a68e23e3 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -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.

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.
> 
> Regards,
> /Bruce
Bruce Richardson Nov. 27, 2019, 9:32 a.m. UTC | #3 
On Wed, Nov 27, 2019 at 02:19:39AM +0000, Yu, Jin wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Tuesday, November 26, 2019 6:00 PM
> > To: Yu, Jin <jin.yu@intel.com>
> > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> > <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> > dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] vhost: fix insecure temporary file
> > 
> > On Tue, Nov 26, 2019 at 11:19:00PM +0800, Jin Yu wrote:
> > > 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: stable@dpdk.org
> > >
> > > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > > ---
> > >  lib/librte_vhost/vhost_user.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/lib/librte_vhost/vhost_user.c
> > > b/lib/librte_vhost/vhost_user.c index 0cfb8b792..1a68e23e3 100644
> > > --- a/lib/librte_vhost/vhost_user.c
> > > +++ b/lib/librte_vhost/vhost_user.c
> > > @@ -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.
> 
> 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.
> > 
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
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 0cfb8b792..1a68e23e3 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -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);
 	}