1491476 - Latent write beyond bounds in nr_transport_addr_fmt_ifname_addr_string()

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[mozillabugs]

nr_transport_addr_fmt_ifname_addr_string() (media\mtransport\third_party\nICEr\src\net\transport_addr.c) will write zeroes beyond bounds over the stack on error. At present, it appears that this error cannot occur, so the bug is latent.

The bug is in lines 107 and 112, which use |len| -- the length of the input/output buffer |buf| -- instead of the length of the temporary output buffer |buffer|. |len| typically is longer than |sizeof(buffer)|, so this error causes strncpy() to write zeroes beyond |buffer|'s end:

	 98: int nr_transport_addr_fmt_ifname_addr_string(const nr_transport_addr *addr, char *buf, int len)
	 99:   {
	100:     int _status;
	101:     /* leave room for a fully-expanded IPV4-mapped IPV6 address */
	102:     char buffer[46];
	103: 
	104:     switch(addr->ip_version){
	105:       case NR_IPV4:
	106:         if (!inet_ntop(AF_INET, &addr->u.addr4.sin_addr,buffer,sizeof(buffer))) {
	107:            strncpy(buffer, "[error]", len);
	108:         }
	109:         break;
	110:       case NR_IPV6:
	111:         if (!inet_ntop(AF_INET6, &addr->u.addr6.sin6_addr,buffer,sizeof(buffer))) {
	112:            strncpy(buffer, "[error]", len);
	113:         }
	114:         break;
	115:       default:
	116:         ABORT(R_INTERNAL);
	117:     }
	118:     buffer[sizeof(buffer) - 1] = '\0';
	119: 
	120:     snprintf(buf,len,"%s:%s",addr->ifname,buffer);
	121:     buf[len - 1] = '\0';
	122: 
	123:     _status=0;
	124:   abort:
	125:     return(_status);
	126:   }

You can force the write beyond bounds by starting FF (preferably a debug build), attaching a debugger to it, and setting a BP on line 106. Now navigate to https://webrtc.github.io/samples/src/content/peerconnection/audio and click "call". Let the site use your (muted!) microphone. When the BP fires, use the debugger to simulate failure of line 106 by manually setting control to line 107. Step line 107 and watch strncpy() write about 34 bytes beyond bounds onto the stack. Step a few times and something (possibly snprintf() on line 120) will crash. Presumably the same approach will work with lines 111-12.


[1] Note that strncpy() doesn't guarantee proper termination in all cases, though it should terminate here, since the source string "[error]" is shorter than |sizeof (buffer)|. strncpy() has my nomination for the worst library function of all time.

Comment 1

[Nils Ohlmeier [:drno]]

Byron, can you please take care of this?

Comment 2

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fb3ce337b087369c24a88040089a3a05e1132a9

Comment 3

[Byron Campen [:bwc]]

Attachment: Bug 1491476: Fix strncpy length in a couple of error cases. r?mjf

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 9009665 [details]
Bug 1491476: Fix strncpy length in a couple of error cases. r?mjf

Nils Ohlmeier [:drno] has approved the revision.

Comment 5

[Michael Froman [:mjf]]

Comment on attachment 9009665 [details]
Bug 1491476: Fix strncpy length in a couple of error cases. r?mjf

Michael Froman [:mjf] has approved the revision.

Comment 6

[Byron Campen [:bwc]]

Marking this one checkin-needed, but would like guidance on the right way to land security bugs when using phabricator.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Byron, can you confirm that this is not sec-critical or sec-high? See https://wiki.mozilla.org/Security/Bug_Approval_Process

Shall this land with the current commit message "Fix strncpy length in a couple of error cases"?

At the moment, to land it I run |arc patch --nobranch D<differential revision>| and then hg commit -e and update the reviewers in the first line and drop the other Phabricator information, especially the security tags.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #7)
> hg commit -e
s/-e/--amend/

Comment 9

[Byron Campen [:bwc]]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #7)
> Byron, can you confirm that this is not sec-critical or sec-high? See
> https://wiki.mozilla.org/Security/Bug_Approval_Process

   This is a latent bug, so it isn't sec-critical or sec-high.

> Shall this land with the current commit message "Fix strncpy length in a
> couple of error cases"?

   Sure.

> At the moment, to land it I run |arc patch --nobranch D<differential
> revision>| and then hg commit -e and update the reviewers in the first line
> and drop the other Phabricator information, especially the security tags.

   Ok, and then you just push to inbound manually? Lando is not involved, nor is phabricator, once you amend the commit message?

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a7893e7f922aa308f80098efe1b450a06f67a8

>    Ok, and then you just push to inbound manually? Lando is not involved,
> nor is phabricator, once you amend the commit message?
For now, yes.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4a7893e7f92