905080 - Uninitialised value use relating to sctp_send_initiate_ack

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Julian Seward [:jseward]]

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)

Comment 1

[Michael Tüxen]

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

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 3

[Michael Tüxen]

... 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...

Comment 4

[Julian Seward [:jseward]]

Attachment: bug905080c4.diff

One-liner: memset(&stc, 0, sizeof(stc));
makes all the Memcheck warnings go away.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 7

[Julian Seward [:jseward]]

(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.

Comment 8

[Michael Tüxen]

(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.

Comment 9

[Michael Tüxen]

(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.

Comment 10

[Michael Tüxen]

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

Comment 11

[Michael Tüxen]

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 ([e-mail]) or
the Mozilla project or anyone else?

Best regards
Michael

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

See Michael's question about who should be listed in FreeBSD's advisory

Comment 13

[Julian Seward [:jseward]]

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.

Comment 14

[Michael Tüxen]

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...

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

This should be fixed by the sctp library update landed in bug 916427

Comment 16

[Al Billings [:abillings - ex-MoCo]]

(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?