Re: advapi32: fix a too small buffer in CredUnmarshalCredentialW [try2]

[Hans Leidekker]

On Wed, 2012-11-14 at 16:28 +0100, Stefan Leichter wrote:
> @@ -2053,6 +2053,8 @@ static BOOL cred_decode( const WCHAR *cred, unsigned int len, char *buf )
>      char c0, c1, c2, c3;
>      const WCHAR *p = cred;
> - TRACE("%s\n", debugstr_wn(cred,len));

This string is already traced in CredUnmarshalCredentialW.
> @@ -2134,6 +2136,7 @@ BOOL WINAPI CredUnmarshalCredentialW( LPCWSTR cred, PCRED_MARSHAL_TYPE type, PVO
>      case UsernameTargetCredential:
>      {
>          USERNAME_TARGET_CREDENTIAL_INFO *target;
> -
>    ULONGLONG size = 0;
>
>    if (len < 9 || !cred_decode( cred + 3, 6, (char *)&size ) || !size || size % sizeof(WCHAR))
>    { 

You should also perform a sanity check on 'size' to avoid overflow in
calculations that follow.

advapi32: fix a too small buffer in CredUnmarshalCredentialW [try2]

[Stefan Leichter]

Wednesday 14 November 2012 Hans Leidekker
> On Wed, 2012-11-14 at 16:28 +0100, Stefan Leichter wrote:
> > @@ -2053,6 +2053,8 @@ static BOOL cred_decode( const WCHAR *cred,
> > unsigned int len, char *buf )
> >  char c0, c1, c2, c3;
> >  const WCHAR *p = cred;
> >
> > - TRACE("%s\n", debugstr_wn(cred,len));
>
> This string is already traced in CredUnmarshalCredentialW.
> > @@ -2134,6 +2136,7 @@ BOOL WINAPI CredUnmarshalCredentialW( LPCWSTR cred,
> > PCRED_MARSHAL_TYPE type, PVO
> >  case UsernameTargetCredential:
> >  {
> >
> >      USERNAME_TARGET_CREDENTIAL_INFO *target;
> >
> > -
> >    ULONGLONG size = 0;
> >
> >    if (len < 9 || !cred_decode( cred + 3, 6, (char *)&size ) ||
> >    !size || size % sizeof(WCHAR)) {
>
> You should also perform a sanity check on 'size' to avoid overflow in
> calculations that follow.

I think it is the best when you start fixing your code yourself

advapi32: fix a too small buffer in CredUnmarshalCredentialW [try2]

[Stefan Leichter]

Wednesday 14 November 2012 Stefan Leichter
> Wednesday 14 November 2012 Hans Leidekker
> > On Wed, 2012-11-14 at 16:28 +0100, Stefan Leichter wrote:
> > > @@ -2053,6 +2053,8 @@ static BOOL cred_decode( const WCHAR *cred,
> > > unsigned int len, char *buf )
> > >  char c0, c1, c2, c3;
> > >  const WCHAR *p = cred;
> > >
> > > - TRACE("%s\n", debugstr_wn(cred,len));
> >
> > This string is already traced in CredUnmarshalCredentialW.
> > > @@ -2134,6 +2136,7 @@ BOOL WINAPI CredUnmarshalCredentialW( LPCWSTR
> > > cred, PCRED_MARSHAL_TYPE type, PVO
> > >  case UsernameTargetCredential:
> > >  {
> > >
> > >      USERNAME_TARGET_CREDENTIAL_INFO *target;
> > >
> > > -
> > >    ULONGLONG size = 0;
> > >
> > >    if (len < 9 || !cred_decode( cred + 3, 6, (char *)&size ) ||
> > >    !size || size % sizeof(WCHAR)) {
> >
> > You should also perform a sanity check on 'size' to avoid overflow in
> > calculations that follow.
>
> I think it is the best when you start fixing your code yourself

Hello Hans,

i have to say sorry, this answer is rude.

But i don't like to do unnecessary iteration on source code especially when 
the "complain" has been in the previous version too. Source code usually does 
not get better from iteration to iteration in this case.

Let me rephrase the last mail:
I will not send a new patch about this topic in the near future because lack 
of time. So its now up to you or anyone else to fix the problem.

Regards Stefan

advapi32: fix a too small buffer in CredUnmarshalCredentialW [try2]

[Hans Leidekker]

On Thu, 2012-11-15 at 09:41 +0100, Stefan Leichter wrote:
> i have to say sorry, this answer is rude.
>
> But i don't like to do unnecessary iteration on source code especially when 
> the "complain" has been in the previous version too. Source code usually does 
> not get better from iteration to iteration in this case.

No offense taken. You second attempt certainly was an improvement, I just
spotted more problems that I should have seen and mentioned in the first review.