54796 - PR_InitializeNetAddr() does not clear trailing garbage

Status: VERIFIED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Shin'ichiro TAYA]

PR_InitializeNetAddr() does not clear addr->sin_zero.
This cause PR_Bind() failure in SSM_OpenPort() at security/psm/server/servutil.c
I think every part which sets address to PRNetAddr should clear whole buffer
before set address.
I also tested on FreeBSD-3.3 and got same result.

Comment 1

[Shin'ichiro TAYA]

Attachment: sample program

Comment 2

[Wan-Teh Chang]

Thanks for the bug report.  We are aware of
this problem of PR_InitializeNetAddr() and
in hindsight we should have had the function
zero the whole PRNetAddr before setting the
members.  We can't change the function to
clear the PRNetAddr because that will change
the behavior of the function.

So callers of PR_InitializeNetAddr will
have to zero the PRNetAddr themselves.
You need to add something like
    memset(&addr, 0, sizeof(PRNetAddr));
to the PSM code before the PR_InitializeNetAddr
call.

Comment 3

[Wan-Teh Chang]

I'm now convinced that this bug should be fixed
in NSPR rather than having all the apps work
around it.

I need to convince myself that no apps may depend
on this bug.

Comment 4

[Ben Bucksch (:BenB)]

rubber-stamp confirm. cluefull reporter. Shin'ichiro TAYA, ask somebody from QA
to increase your rights.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch.

Comment 6

[Wan-Teh Chang]

John, I'd like you to review my patch (id=23603).

1. I only zero the sock addr when the 'val' argument
   to PR_InitializeNetAddr or PR_SetNetAddr is not
   PR_IpAddrNull because in the PR_IpAddrNull case
   we need to keep the IP address field intact.

2. I could zero the whole PRNetAddr, but I only zero
   the PRNetAddr union member in use for two reasons:
   - PRNetAddr contains the Unix domain sock addr on
     Unix, which is 108 bytes.  In contract, the 'inet'
     member is only 16 bytes and the 'ipv6' member is
     only 32 bytes.
   - I am concerned that some existing code may actually
     pass a sockaddr_in structure in, so I am concerned
     about writing beyond the end of the sockaddr_in structure.
     This is probably not worth worrying about.

I am very concerned that this patch breaks binary compatibility
because PR_InitializeNetAddr and PR_SetNetAddr is not zeroing
the sock addr right now.  What do you think?  Do you think we
should modify PR_InitializeNetAddr to zero the sock addr, or
ask NSPR clients to zero the sock addr before passing it to
PR_InitializeNetAddr (essentially working around this bug)?

Comment 7

[John G. Myers]

Attachment: I think this would be a cleaner fix

Comment 8

[John G. Myers]

I think we should apply the fix.  It is extremely unlikely existing NSPR clients 
depend on current behavior.  Such dependence would be an incorrect use of the 
published API.

Comment 9

[Wan-Teh Chang]

Attachment: To use John's patch, I need to move the setting of 'family' after the memset call.

Comment 10

[Wan-Teh Chang]

Attachment: Removed extra '&' in front of 'addr' in previous patch.

Comment 11

[Wan-Teh Chang]

I checked in my latest patch (id=23614)
on the tip, the NSPRPUB_RELEASE_4_1_BRANCH,
and the NSPRPUB_CLIENT_BRANCH.

I found that PR_StringToNetAddr should also
zero the sock addr first, but the code in
that function is more complicated.  If the
caller of PR_StringToNetAddr always passes
a PRNetAddr as they should, there is a simple
patch which I will attach next.

Comment 12

[Wan-Teh Chang]

On second thought, I'll open a separate bug
report for PR_StringToNetAddr so that I can
mark this bug fixed.

Comment 13

[Wan-Teh Chang]

*** Bug 65549 has been marked as a duplicate of this bug. ***

Comment 14

[Shin'ichiro TAYA]

With this fix, I could build & run psm on NetBSD/i386-current.

BTW, two problems building psm.
While I'm building psm,
1)NSS didn't compile
I compiled it manually.
2)psm was installed to dist/NetBSD1.5Q_DBG.OBJ/bin.
Mozilla failed to run psm. Because mozilla launch dist/bin/start-psm.

Is it a NetBSD specific problem?

Anyway, this bug has fix with your work.
Thank you!

Comment 15

[Wan-Teh Chang]

Hi Taya,

Thanks for verifying that the fix works.

Unfortunately, I am not building PSM now, so
I won't be able to help you with the PSM/NSS
build problems.  I suggest that you post a
question to mozilla.crypto.