Closed Bug 332006 Opened 19 years ago Closed 2 years ago

Avoid raw calls to snprintf

Categories

(Core :: General, defect, P3)

Product:
Core
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: darin.moz, Assigned: tjr)

References

Details

(Keywords: sec-want)

Attachments

(10 files)

Proposed patch
(deleted), patch
dougt
: review+
Details | Diff | Splinter Review
Patch1.diff
(deleted), patch
Details | Diff | Splinter Review
Patch2.diff
(deleted), patch
Details | Diff | Splinter Review
Patch3.diff
(deleted), patch
Details | Diff | Splinter Review
Patch4.diff
(deleted), patch
Details | Diff | Splinter Review
Patch5.diff
(deleted), patch
Details | Diff | Splinter Review
Patch6.diff
(deleted), patch
Details | Diff | Splinter Review
WIP: Bug 332006: Replace _snprintf with snprintf
(deleted), text/x-phabricator-request
Details
Bug 332006: Replace _snprintf with snprintf for expected and consistent behavior on Windows r?gsvelto
(deleted), text/x-phabricator-request
Details
Bug 332006: Remove an old cairo patch r?jrmuizel
(deleted), text/x-phabricator-request
Details
Avoid raw calls to snprintf snprintf does not promise to null terminate the result buffer. any instance of snprintf in the tree that can use PR_snprintf should do so, and any that cannot should be replaced by a thin wrapper function that ensures a null terminated result. such a function could be provided by the xpcom glue or something like that. we probably have a similar problem in the updater, which does not link to nspr or the xpcom glue.
This is marked as OS=all, but at least on Solaris this is not true. From snprintf(3c): The sprintf() function places output, followed by the null byte (\0), in consecutive bytes starting at s; it is the user's responsibility to ensure that enough storage is available. The snprintf() function is identical to sprintf() with the addition of the argument n, which specifies the size of the buffer referred to by s. If n is 0, nothing is written and s can be a null pointer. Otherwise, output bytes beyond the n-1st are discarded instead of being written to the array and a null byte is written at the end of the bytes actually written into the array.
Not surprisingly, you'll find similar language in the Single Unix Specification version 3 from opengroup.org. The null-termination feature of snprintf is set by standard. If there's a platform out there that gets this wrong, I'd call that a bug in the platform, not a "feature" that applications ought to try to work around.
Yup, there are definitely sane snprintf implementations out there.
> that a bug in the platform, not a "feature" that applications ought to try to > work around. You can call it whatever you want, but if we know that some platforms get this wrong it'd still be nice to avoid security problems on them.
Since we are a cross-platform app and the MSVC stdlib is known to get this wrong, we need to fix it whether or not it's "our" bug.
Component: XPCOM → General
Keywords: helpwanted
Priority: -- → P3
Whiteboard: [mentor=bsmedberg][lang=c++]
Hello, my friend and I would like to help out Mozilla, and we believe this is a good bug for us to start with. Is this bug still open? And if so, what are the first steps we should to get started.
Assignee: nobody → smartaled
We've worked around this problem in other places. It's fairly simple to write a snprintf implementation for Windows that does the right thing: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_stdio.c#70 Finding and fixing all the places where we call snprintf() is probably the hard part. I'm not sure what the best solution would be.
Attached patch Proposed patch (deleted) — — Splinter Review
Not the full patch, just a submission for review of correctness
I have been looking into the cases where snprintf is used in the tree, and marking which can use NSPR and which cant, thanks to multiple members on irc. I just wanted to make sure that when I can use NSPR, that its a simple switch from snprintf to PR_snprintf as the patch shows. And when you can't use NSPR such as the multitude of instances in media/webrtc/signaling/src/sipcc/core/sipstack, what should be done.
Comment on attachment 718177 [details] [diff] [review] Proposed patch Yes, I believe this is the right way. Having small patches like this is a great way to start getting stuff landed. In this case you should request review from one of the people who has been reviewing dom/camera code already; probably :dougt
I should note (having been down this road with media/webrtc/signaling) that many apparent uses of "snprintf" in the tree are actually aliased via #define's to internal snprintf replacements.
Comment on attachment 718177 [details] [diff] [review] Proposed patch Let's follow up on Benjamin's suggestion here.
Attachment #718177 - Flags: review?(doug.turner)
Comment on attachment 718177 [details] [diff] [review] Proposed patch dougt review-ping?
Comment on attachment 718177 [details] [diff] [review] Proposed patch just to confirm -- Isn't this only needed for windows now? I think the mac and linux have real implementations of snprintf.
Attachment #718177 - Flags: review?(doug.turner) → review+
Edwin, are you able to create further patches for the remaining instances, now that this one has been approved?
Flags: needinfo?(smartaled)
Could we land what we already have here?
Keywords: sec-want
Is this bug still active?
Flags: needinfo?(josh)
Yep. Feel free to give the patch here some love, or continue the fine work.
Flags: needinfo?(smartaled)
Flags: needinfo?(josh)
Attached patch Patch1.diff (deleted) — — Splinter Review
Assignee: smartaled → anujagarwal464
Attachment #8416566 - Flags: review?(dougt)
Attached patch Patch2.diff (deleted) — — Splinter Review
Attachment #8416568 - Flags: review?(dougt)
Attached patch Patch3.diff (deleted) — — Splinter Review
Attachment #8416569 - Flags: review?(dougt)
Attached patch Patch4.diff (deleted) — — Splinter Review
Attachment #8416570 - Flags: review?(dougt)
Attached patch Patch5.diff (deleted) — — Splinter Review
Attachment #8416571 - Flags: review?(dougt)
Attached patch Patch6.diff (deleted) — — Splinter Review
Attachment #8416572 - Flags: review?(dougt)
Can we instead do something with gcc/link magic - that is substitute all call sites with a safe version (depending on their platform). That would allow new-developers-to-gecko to just use snprintf and the safe/right thing happens. Assuming we can't, can we undefine snprintf in gecko code so that the build breaks if a new instance occurs in the tree?
Flags: needinfo?(benjamin)
> #ifdef XP_WIN > // we want a wmain entry point > #include "nsWindowsWMain.cpp" >-#define snprintf _snprintf >+#define PR_snprintf _snprintf I don't think this blind search-and-replace is correct.
Also I thought we were going to reduce dependency on NSPR rather than increasing. sprintf_s is usable on Windows. According to comment #1 and #2, snprintf is enough safe on other platforms.
I believe snprintf is safe on non-Windows because it always null-terminates. The problem is only on Windows, and I don't know of any linker magic to reliably fix this there. I do wonder whether we could use #define magic there. I don't know whether we can poison it or whether that will kill system headers.
Flags: needinfo?(benjamin)
sprintf_s is not equivalent to snprintf, per the docs: " If the buffer is too small for the text being printed then the buffer is set to an empty string and the invalid parameter handler is invoked." http://msdn.microsoft.com/en-us/library/ce3zzk1k.aspx I wrote a Windows snprintf a while back when porting some WebRTC code: http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/util.c#753 We could put that in mozglue or somewhere and use it, and then wipe the codebase clean of anyone using _snprintf (which is unsafe).
Do we need the same behavior on all platforms even if the buffer is too small? It is a bug itself.
I'm not aware of any other platform that gets this wrong, only Windows. The POSIX spec says that the function should truncate and null-terminate if the buffer is too small.
Then can callers depend on the truncation and zero-terminate? If any existing code is depending on that, it is already broken on Windows because the current polyfill (#define sprintf _sprintf) will not handle a too small buffer correctly. sprintf_s would make the undefined behavior to a defined one, at least.
Yes, as far as I'm aware callers of snprintf (not _snprintf) can depend on the null-termination behavior. sprintf_s would certainly turn a potential security hole (not null terminating the string) into a safe crash, but I don't think that's the ideal solution. It's less bad, but the snprintf implementation from comment 30 (or comment 7) replicates the POSIX behavior.
Attachment #8416566 - Flags: review?(dougt)
Attachment #8416568 - Flags: review?(dougt)
Attachment #8416569 - Flags: review?(dougt)
Attachment #8416570 - Flags: review?(dougt)
Attachment #8416571 - Flags: review?(dougt)
Attachment #8416572 - Flags: review?(dougt)
Mentor: benjamin
Whiteboard: [mentor=bsmedberg][lang=c++] → [lang=c++]
Assignee: anujagarwal464 → nobody
Hi I am looking for my first bug to work on. Where can I help with this bug?
Theres mfbt/Sprintf.h now. Calls like snprintf(buffer, sizeof(buffer), ...) can use SprintfLiteral instead, for some extra safety. Unfortunately there are many calls not of this form.
Mentor: benjamin
QA Whiteboard: qa-not-actionable

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --
Keywords: helpwanted
Whiteboard: [lang=c++]

No direct user impact.

Severity: -- → N/A

The bad snprintf behavior on Windows was done by _snprintf; and the new (2015) snprintf function does the correct thing. So I think it would be sufficient for this bug not to cut over all the snprintf calls to some other snprintf, but rather fix the fewer snprintf -> _snprintf defines.

This seems broad enough that it can stay in Core:General.

Severity: N/A → S3

The attached patch seems to build and run - I don't think we can land it as is, but I bet we can land individual components of it that will address the majority of instances.

Assignee: nobody → tom
Status: NEW → ASSIGNED

So it turns out that the vast majority of changes are upstream libraries so.... that's difficult. But here's two in our code.

Keywords: leave-open

Do we still build on MSC_VER < 1900 (pre VS2015)? I'm guessing not...
We'll need to leave imported code (and comments, etc) alone, I think.
Unconditional defines to _snprintf could be removed, probably (or if building on MSC_VER<1900 is still viable, adding a #if). Ditto for "#if MSC_VER"

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #46)

Do we still build on MSC_VER < 1900 (pre VS2015)? I'm guessing not...

We dropped support for pre-VS2005u3. See bug 1319901.

We'll need to leave imported code (and comments, etc) alone, I think.

We can touch dom/media/webrtc/transport/third_party/nrappkit/upstream.diff. This file is our local patch, not imported code. Here is a precedent:
https://hg.mozilla.org/mozilla-central/rev/ea36b327a1ee18c08613d1fe36cc6b8d18c8e47f

Similarly, we can add a .patch file under gfx/cairo/ and apply the patch to cairo-compiler-private.h.

Because cairo's snprintf -> _snprintf define is under a _MSC_VER < 1900 conditional we don't need to do anything there.

And dom/media/webrtc/transport/third_party/nrappkit/upstream.diff is actually already doing the correct thing - it's removing the snprintf -> _snprintf mapping from the upstream code.

gfx/cairo/win32-inline-cpp-keyword.patch seems like a old dead patch that wouldn't apply

ipc/chromium/src/third_party/libevent/sample/https-client.c isn't built

third_party/libsrtp/src/test/cutest.h is a test file

third_party/wasm2c/src/config.cc and third_party/wasm2c/src/config.h.in is acknowledging the problem and making a work-around

other-licenses/nsis/Contrib/ExDLL/tchar.h which I'm not worried about because this is not client code

These are all also fine (behind a _MSC_VER < 1900 conditional)

  • gfx/angle/checkout/src/common/angleutils.h
  • gfx/harfbuzz/src/hb.hh
  • media/libvpx/libvpx/vpx_ports/msvc.h
  • modules/freetype2/src/gzip/gzguts.h
  • modules/zlib/src/gzguts.h
  • nsprpub/pr/src/io/prprf.c
  • security/nss/lib/zlib/gzguts.h (so many zlibs!)
  • third_party/aom/aom_ports/msvc.h
  • third_party/libwebrtc/rtc_base/logging.cc
  • toolkit/crashreporter/google-breakpad/src/common/stdio_wrapper.h

That leaves the following upstream code:

  • gfx/angle/checkout/third_party/vulkan-deps/vulkan-headers/src/include/vulkan/vk_sdk_platform.h
  • two things in gfx/skia
  • toolkit/components/protobuf/src/google/protobuf/stubs/strutil.cc
  • toolkit/components/protobuf/src/google/protobuf/stubs/common.cc
  • toolkit/crashreporter/google-breakpad/src/third_party/libdisasm/ things
Attachment #9298699 - Attachment description: Bug 332006: Replace _snprintf with snprintf for expected and consistent behavior on Windows r?gsvelto,cmartin → Bug 332006: Replace _snprintf with snprintf for expected and consistent behavior on Windows r?gsvelto

This patch hasn't been touched in ten years and the code it affects does not
appear in the file, so I think it is old an unused.

Depends on D159430

Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/322b89959ff7 Replace _snprintf with snprintf for expected and consistent behavior on Windows r=gsvelto,cmartin https://hg.mozilla.org/integration/autoland/rev/e91e120034b7 Remove an old cairo patch r=jrmuizel

gfx/angle/checkout/third_party/vulkan-deps/vulkan-headers/src/include/vulkan/vk_sdk_platform.h doesn't seem to be included anywhere, nor does any vulkan file use snprintf.

toolkit/components/protobuf/src/google/protobuf/stubs/strutil.cc is safe, there's a bunch of extra checks for it. Plus I don't think it's actually accessible by web content.

toolkit/components/protobuf/src/google/protobuf/stubs/common.cc is similarly safe

For libdisasm, AFAICT libdisasm is only used by the exploitability check, and we disable that check. So we might be able to remove that entire blob of code. I filed Bug 1798516 for that.

This just leaves Skia.

I filed https://bugs.chromium.org/p/skia/issues/detail?id=13879 upstream, but looking closer, skia only seems to use it for two things: path debugging (not relevant) and determining the length of the string to write to a stream (write_scalar) - the latter doesn't need to worry about a null terminator.

So I think we can call this fixed.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: