855335 - Audit SIPCC printf-style formatting strings

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P3)

Description

[Adam Roach [:abr]]

Compiling the SIPCC code with log macros expanded directly to printf() yields approximately 300 formatting-related warnings. Most of these are benign, but some could be problematic. This bug represents an audit task to examine and fix (or suppress) these warnings.

Comment 1

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

marking s-s since we expect there will be some log messages that invoke memory errors.  Note however that any security issue would only apply if logging is turned on, which means debug builds or private builds typically, unless the module is forcing PR_LOGGING on.

Likely anything found would be sec-low or at most sec-moderate for that reason

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Format Warnings from Signaling code

Output of the format-related warnings in the signaling code of Mozilla-Central as of today - rev 7c3cb087620b

Comment 3

[Ethan Hugg [:ehugg]]

Looking at this list I see a bunch that could cause crashes in the future.  

I'm going to take this bug and start with a patch that resolves the -Wformat warnings.  After that we can tackle the -Wformat-security and -Wformat-extra-args

Comment 4

[Adam Roach [:abr]]

(In reply to Ethan Hugg [:ehugg] from comment #3)
> Looking at this list I see a bunch that could cause crashes in the future.  
> 
> I'm going to take this bug and start with a patch that resolves the -Wformat
> warnings.  After that we can tackle the -Wformat-security and
> -Wformat-extra-args

Excellent, thanks.

I would propose that the first step is to run debugging all the way up to 9, run a text call, and then fix the first message that causes a segfault. Repeat as necessary.

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: Patch for testing only. Sends logging to printf

Comment 6

[Ethan Hugg [:ehugg]]

Attachment: Signaling - fix Wformat warnings in logging

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Signaling - fix Wformat warnings in logging

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 735936 [details] [diff] [review]
Signaling - fix Wformat warnings in logging


This should fix all of the -Wformat warnings, but not the -Wformat-extra-args or -Wformat-security ones.

You can now run with NSPR_LOG_MODULES=signaling:9 without crashing.

There is a test patch above that redefines the logging to printf if you want to check the warnings on your build.

Comment 9

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/146c6ca0aa6d

Comment 10

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

Assuming from comment 8 that this is leave-open for now

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/146c6ca0aa6d

Comment 12

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

reopening since this is [leave-open]

Comment 13

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Signaling fix warnings format-extra-args and format-security

Comment 14

[Ethan Hugg [:ehugg]]

Comment on attachment 736506 [details] [diff] [review]
Patch 2 - Signaling fix warnings format-extra-args and format-security

Review of attachment 736506 [details] [diff] [review]:
-----------------------------------------------------------------

This fixes all of the -Wformat-extra-args warnings and all of the -Wformat-security warnings except the ones in fsmcnf.c and fsmdef.c which are caused by the format parameter being the return of another function (get_debug_string).  I chose not to address those since that would entail getting replacing all of the get_debug_string calls with literals.

I don't think this patch fixes anything that would cause a crash, those were fixed in the first patch.

Comment 15

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16b7c4a657df

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16b7c4a657df

Sorry about that, don't know how I missed the [leave open].

Comment 17

[Timothy B. Terriberry (:derf)]

(In reply to Adam Roach [:abr] from comment #0)
> Compiling the SIPCC code with log macros expanded directly to printf()
> yields approximately 300 formatting-related warnings. Most of these are

BTW, you know you can get gcc to generate these warnings without such hackery, right? See the "format" attribute at http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html