[PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

Because the string is zeroed before, it's better to just copy one character less, instead of using strlcpy in this case.

This was found using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 fs/cifs/cifsencrypt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 4934347..dcca8b3 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
 
 	memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
 	if (password)
-		strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
+		strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
 
 	if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) {
 		memcpy(lnm_session_key, password_with_pad,
-- 
1.7.10.4

[PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

If you are going to use memset before strncpy you must copy sizeof -1

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 fs/cifs/cifsencrypt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 4934347..dcca8b3 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
 
 	memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
 	if (password)
-		strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
+		strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
 
 	if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) {
 		memcpy(lnm_session_key, password_with_pad,
-- 
1.7.10.4

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Shirish Pargaonkar]

Acked-by: Shirish Pargaonkar <spargaonkar@suse.com>

On Sat, Jul 26, 2014 at 5:28 PM, Rickard Strandqvist
<rickard_strandqvist@spectrumdigital.se> wrote:
> If you are going to use memset before strncpy you must copy sizeof -1
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  fs/cifs/cifsencrypt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 4934347..dcca8b3 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
>
>         memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
>         if (password)
> -               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
> +               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
>
>         if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) {
>                 memcpy(lnm_session_key, password_with_pad,
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Joe Perches]

On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote:
> Acked-by: Shirish Pargaonkar <spargaonkar@suse.com>
[]
> > diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
[]
> > @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
> >
> >         memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
> >         if (password)
> > -               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
> > +               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);


Is this always correct?

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Rickard Strandqvist]

2014-08-02 19:33 GMT+02:00 Joe Perches <joe@perches.com>:
> On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote:
>> Acked-by: Shirish Pargaonkar <spargaonkar@suse.com>
> []
>> > diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> []
>> > @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
>> >
>> >         memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
>> >         if (password)
>> > -               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
>> > +               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
>
>
> Is this always correct?


Hi

Because password_with_pad gets set to all zeros above, the character,
I do not guarantee a copy terminating null.
Unless it is so that you do not want any terminating null.


Kind regards
Rickard Strandqvist

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Joe Perches]

On Sun, 2014-08-03 at 01:13 +0200, Rickard Strandqvist wrote:
> 2014-08-02 19:33 GMT+02:00 Joe Perches <joe@perches.com>:
> > On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote:
> >> Acked-by: Shirish Pargaonkar <spargaonkar@suse.com>
> > []
> >> > diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> > []
> >> > @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
> >> >
> >> >         memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
> >> >         if (password)
> >> > -               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
> >> > +               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
> >
> >
> > Is this always correct?
[]
> Because password_with_pad gets set to all zeros above, the character,
> I do not guarantee a copy terminating null.
> Unless it is so that you do not want any terminating null.

That's the question for Steve and cifs people.

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

[Shirish Pargaonkar]

Joe, Thanks for watching out.

I think this patch is not correct and think also the current LM
authentication code is broken
if the password length exceeds 14 characters.

Reading from Chris Hertel's book (Chapter 15, 15.3.3 Creating the LM
Hash), it says:

The LM Hash is a sixteen byte string, created as follows:

1. The password, as entered by the user, is either padded with nuls
(0x00) or trimmed
    to fourteen (14) bytes.
    * Note that the 14-byte password string is not handled as
null-terminated string.  If the
      user enters 14 or more bytes, the last byte in the modified
string will not be nul.
    * Also note the password is given in the 8-bit OEM character set
(extended ASCII),
      not Unicode.
2. The 14-byte password string is converted to all uppercase.

and so on....


So I thought a (quick) patch like this should be a fix instead but it
does not work against a
Samba server if the strlen(password) is more than 14.

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 4934347..aecb4a0 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -307,7 +307,8 @@ int calc_lanman_hash(const char *password, const char *crypt

        memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
        if (password)
-               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
+               strncpy(password_with_pad, password,
+                       min_t(unsigned int, strlen(password), 14));

        if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) {
                memcpy(lnm_session_key, password_with_pad,

So there is work to be done in lanman auth code but this patch should
not be included.
So taking back the ack.


On Sat, Aug 2, 2014 at 7:27 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2014-08-03 at 01:13 +0200, Rickard Strandqvist wrote:
>> 2014-08-02 19:33 GMT+02:00 Joe Perches <joe@perches.com>:
>> > On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote:
>> >> Acked-by: Shirish Pargaonkar <spargaonkar@suse.com>
>> > []
>> >> > diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
>> > []
>> >> > @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
>> >> >
>> >> >         memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
>> >> >         if (password)
>> >> > -               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
>> >> > +               strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
>> >
>> >
>> > Is this always correct?
> []
>> Because password_with_pad gets set to all zeros above, the character,
>> I do not guarantee a copy terminating null.
>> Unless it is so that you do not want any terminating null.
>
> That's the question for Steve and cifs people.
>