Uninitialised value use relating to sctp_send_initiate_ack

RESOLVED FIXED in mozilla30

Status

()

Core
WebRTC: Networking
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: jseward, Assigned: Michael Tüxen, NeedInfo)

Tracking

({sec-low})

Trunk
mozilla30
x86_64
Linux
sec-low
Points:
---

Firefox Tracking Flags

(firefox-esr24 wontfix)

Details

(Whiteboard: [webrtc])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
TEST_PATH=dom/media/tests/mochitest/test_dataChannel_basicAudio.html

It appears that sctp_send_initiate_ack is creating uninitialised
values which are then copied to various different places, and result
in multiple Memcheck error reports.

Thread 3:
Use of uninitialised value of size 8
   at 0x58422C9: sctp_calculate_cksum (sctp_crc32.c:555)
   by 0x585F821: sctp_lowlevel_chunk_output (sctp_output.c:4808)
   by 0x586B084: sctp_send_initiate_ack (sctp_output.c:6295)
   by 0x584BE38: sctp_handle_init (sctp_input.c:217)
   by 0x5851BA8: sctp_process_control (sctp_input.c:4887)
   by 0x5855EE9: sctp_common_input_processing (sctp_input.c:5907)
   by 0x5891E30: usrsctp_conninput (user_socket.c:3189)
   by 0x58929E6: mozilla::DataChannelConnection::SctpDtlsInput(mozilla::TransportFlow*, unsigned char const*, unsigned long) (DataChannel.cpp:654)
   by 0x5891FC7: sigslot::_connection3<mozilla::MediaPipeline, mozilla::TransportLayer*, unsigned char const*, unsigned long, sigslot::single_threaded>::emit(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:1944)
   by 0x700FCD7: mozilla::TransportFlow::PacketReceived(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:2477)
   by 0x5891FC7: sigslot::_connection3<mozilla::MediaPipeline, mozilla::TransportLayer*, unsigned char const*, unsigned long, sigslot::single_threaded>::emit(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:1944)
   by 0x70185CD: sigslot::signal3<mozilla::TransportLayer*, unsigned char const*, unsigned long, sigslot::single_threaded>::operator()(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:2477)

Use of uninitialised value of size 8
   at 0x58422D8: sctp_calculate_cksum (sctp_crc32.c:558)
   by 0x585F821: sctp_lowlevel_chunk_output (sctp_output.c:4808)
   by 0x586B084: sctp_send_initiate_ack (sctp_output.c:6295)
   by 0x584BE38: sctp_handle_init (sctp_input.c:217)
   by 0x5851BA8: sctp_process_control (sctp_input.c:4887)
   by 0x5855EE9: sctp_common_input_processing (sctp_input.c:5907)
   by 0x5891E30: usrsctp_conninput (user_socket.c:3189)
   by 0x58929E6: mozilla::DataChannelConnection::SctpDtlsInput(mozilla::TransportFlow*, unsigned char const*, unsigned long) (DataChannel.cpp:654)
   by 0x5891FC7: sigslot::_connection3<mozilla::MediaPipeline, mozilla::TransportLayer*, unsigned char const*, unsigned long, sigslot::single_threaded>::emit(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:1944)
   by 0x700FCD7: mozilla::TransportFlow::PacketReceived(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:2477)
   by 0x5891FC7: sigslot::_connection3<mozilla::MediaPipeline, mozilla::TransportLayer*, unsigned char const*, unsigned long, sigslot::single_threaded>::emit(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:1944)
   by 0x70185CD: sigslot::signal3<mozilla::TransportLayer*, unsigned char const*, unsigned long, sigslot::single_threaded>::operator()(mozilla::TransportLayer*, unsigned char const*, unsigned long) (sigslot.h:2477)
 Uninitialised value was created by a stack allocation
   at 0x586A486: sctp_send_initiate_ack (sctp_output.c:5652)

(and a whole bunch more of the above, followed by a bunch of the following)

Use of uninitialised value of size 8
   at 0x115788E4: rijndael_encryptBlock128 (rijndael.c:577)
   by 0x11579458: rijndael_encryptCBC (rijndael.c:875)
   by 0x11579F13: AES_Encrypt (rijndael.c:1237)
   by 0x17B1269A: NSC_EncryptUpdate (pkcs11c.c:1000)
   by 0x4CD04C9: PK11_CipherOp (pk11cxt.c:700)
   by 0x4DDE1D8: ssl3_CompressMACEncryptRecord (ssl3con.c:2424)
   by 0x4DDAD82: dtls_CompressMACEncryptRecord (dtlscon.c:803)
   by 0x4DDE590: ssl3_SendRecord (ssl3con.c:2635)
   by 0x4DDE956: ssl3_SendApplicationData (ssl3con.c:2775)
   by 0x4DEE1C2: ssl_SecureSend (sslsecur.c:1304)
   by 0x4DF0BB1: ssl_Send (sslsock.c:2139)
   by 0x4C49868: PR_Send (priometh.c:194)
 Uninitialised value was created by a stack allocation
   at 0x586A486: sctp_send_initiate_ack (sctp_output.c:5652)
(Assignee)

Comment 1

4 years ago
Any idea where the 8 bytes belong to? From looking at the code, we allocate
struct sctp_state_cookie stc on the stack. Some bytes are left uninitialized,
since they are not used. That shouldn't be a problem. To test this, you can
add at the beginning of sctp_send_initiate_ack() the following line:
memset(&stc, 0, sizeof(struct sctp_state_cookie));
If that suppresses the warning, fine. However, it should not be an issue.
Are you able to test the above change?

Best regards
Michael
stc is copied into m; if stc has uninitialized bytes then m will, and the crc for m (the packet) is now calculated across uninitiated bytes which causes the triggers.  This will trigger valgrind and valgrind-like tools, and is also in theory a potential information leak (albeit small) since these parts of stc are sent to a peer.  This is a minor privacy/sec issue.  (perhaps I'm being overly paranoid, in which case we can re-open it.)

memset should do nicely (or my preference: memset(&stc, 0, sizeof(stc)) - habit, rely on the compiler to know what the size/type of something is.  Avoids silly mistakes like memset(&ptr, 0, sizeof(thing_pointed_to)) and memset(foo, 0, sizeof(type_foo_used_to_be)).   You still have to be careful to use memset(ptr, 0, sizeof(*ptr)); the rule is & on the first, OR * on the second.  But this is a personal style thing.  But I've seen a lot of those first two errors over time.
Assignee: nobody → tuexen
Group: media-core-security
Keywords: sec-low
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [webrtc]
(Assignee)

Comment 3

4 years ago
... I do see that we leave some stuff uninitialized in stc. I just wanted to make
sure that we know that it is stc. I'm currently looking into which fields are left
uninitialized in which case. Just wanted to be sure that it is actually stc, not
another stack variable... How can I check?
I do see some bytes uninitialized, but not 8 consecutive ones...
OS: All → Linux
Hardware: All → x86_64
(Reporter)

Comment 4

4 years ago
Created attachment 790211 [details] [diff] [review]
bug905080c4.diff

One-liner: memset(&stc, 0, sizeof(stc));
makes all the Memcheck warnings go away.
Aha.  Size 8 would indicate an x64 pointer or integer, but in this case it may mean that one or more bits/bytes in the 8-byte value loaded was uninitialized
(In reply to Julian Seward from comment #4)
> Created attachment 790211 [details] [diff] [review]
> bug905080c4.diff
> 
> One-liner: memset(&stc, 0, sizeof(stc));
> makes all the Memcheck warnings go away.

Sounds good!  We can take it locally and when it goes in upstream just let that erase our local fix.
(Reporter)

Comment 7

4 years ago
(In reply to Randell Jesup [:jesup] from comment #5)
> Aha.  Size 8 would indicate an x64 pointer or integer, but in this case it
> may mean that one or more bits/bytes in the 8-byte value loaded was
> uninitialized

Ah, sorry.  I just realised that the "uninitialised value of size 8" message
might have been confusing.  What this really means is that a memory address
(for a load or store instruction) was found to contain at least on undefined
bit.  As you say, it doesn't imply that the undefined part of the struct was
8 bytes long.
(Assignee)

Comment 8

4 years ago
(In reply to Julian Seward from comment #4)
> Created attachment 790211 [details] [diff] [review]
> bug905080c4.diff
> 
> One-liner: memset(&stc, 0, sizeof(stc));
> makes all the Memcheck warnings go away.

Great, thanks for the information. This means that I'm focusing on the right code
to fix.
(Assignee)

Comment 9

4 years ago
(In reply to Randell Jesup [:jesup] from comment #6)
> (In reply to Julian Seward from comment #4)
> > Created attachment 790211 [details] [diff] [review]
> > bug905080c4.diff
> > 
> > One-liner: memset(&stc, 0, sizeof(stc));
> > makes all the Memcheck warnings go away.
> 
> Sounds good!  We can take it locally and when it goes in upstream just let
> that erase our local fix.

Whatever you prefer. I guess I'll check in a fix (initializing the missing
components instead of zeroing everything) later today. I'll let you know.
(Assignee)

Comment 10

4 years ago
Fixed upstream in
http://code.google.com/p/sctp-refimpl/source/detail?r=8563
I have added code to explicitly set all components and also
zero out the whole structure since we currently fill the
identification buffer not completely and the time stamp
component contains some padding on all platforms.

Thanks a lot for reporting the issue!

Best regards
Michael
(Assignee)

Comment 11

4 years ago
On the FreeBSD kernel stack (which also uses the same code base for SCTP)
there are two times 4 bytes of kernel memory leaked by this bug.
A corresponding fix has been committed there and will make it into
the upcoming FreeBSD 9.2.
The FreeBSD project is preparing a Security Advisory regarding this.
Who should be listed in credits?  Julian Seward (jseward@acm.org) or
the Mozilla project or anyone else?

Best regards
Michael
See Michael's question about who should be listed in FreeBSD's advisory
Flags: needinfo?(jseward)
Flags: needinfo?(dveditz)
(Reporter)

Comment 13

4 years ago
I am happy to be listed in the advisory if that's the convention, but
actually I have no idea what the conventions are in this kind of case.
Maybe dveditz can advise.
Flags: needinfo?(jseward)
(Assignee)

Comment 14

4 years ago
The FreeBSD sec team will finalize the advisory on Monday, so if there
is no additional information until that point of time,
Julian Seward and myself will be listed in the 'Credits'.
If that is not OK, let me know soon...
This should be fixed by the sctp library update landed in bug 916427
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Group: media-core-security → core-security
(In reply to Randell Jesup [:jesup] from comment #15)
> This should be fixed by the sctp library update landed in bug 916427

Are we going to land this anywhere else?
status-firefox-esr24: --- → wontfix

Updated

2 years ago
Group: core-security → core-security-release

Updated

11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.