2013230 - Crash in mozilla::PrintfTarget::cvt_s when buffer is not zero terminated, because it uses strlen

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Kai Engert [:KaiE:]]

I got SIGSEGV in MOZ_LOG. I used a format string that contains "%.*s" - a string with a precision - to limit the maximum buffer of characters that are printed.

The input is a buffer that is not zero terminated.
I assumed that specifying the precision would be sufficient to avoid reading beyond the end of the buffer.

However, mozilla::PrintfTarget::cvt_s calls strlen. I think it must not do that.

mozilla::PrintfTarget::cvt_s intends to check for an end of the string that is ealier than the specified precision.

IMHO, the function should use strnlen instead, to limit the number of characters checked.

Comment 1

[Kai Engert [:KaiE:]]

Are we worried this could be exploited?

If not, feel free to open the bug.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Bug 2013230 - Use strnlen in mozilla::PrintfTarget::cvt_s. r=glandium

Comment 3

[Daniel Veditz [:dveditz]]

I doubt this could happen in practice (without being caught in development, as you instance was), and moz-log() is off by default. sec-low at best?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6d38f96ad4fe
https://hg.mozilla.org/integration/autoland/rev/0f3d636092d6
Use strnlen in mozilla::PrintfTarget::cvt_s. r=glandium

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5a51f0e21f21
https://hg.mozilla.org/integration/autoland/rev/3725039fbc4d
Revert "Bug 2013230 - Use strnlen in mozilla::PrintfTarget::cvt_s. r=glandium" for causing lint failures @ Printf.cpp

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing lint failures @ Printf.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/3725039fbc4d91b69ff17f2937c920c1324e1d9c

Push with failures

Failure log -> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mozglue/misc/Printf.cpp:22 | /builds/worker/checkouts/gecko/mozglue/misc/Printf.cpp includes <string.h> but does not reference any of its API (includes)

Comment 7

[Mike Hommey [:glandium]]

strnlen is missing in tools/lint/includes/std.py

Comment 8

[[:sergesanspaille]]

Which should be fixed by bug 2016335

Comment 9

[Kai Engert [:KaiE:]]

IIUC it should be fine if we land it again.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a6715fe9ef76
https://hg.mozilla.org/integration/autoland/rev/bfee9cbe224e
Use strnlen in mozilla::PrintfTarget::cvt_s. r=glandium

Comment 11

[Norisz Fay [:noriszfay]]

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

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:KaiE, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Bug 2013230 - Use strnlen in mozilla::PrintfTarget::cvt_s.

Original Revision: https://phabricator.services.mozilla.com/D280910

Comment 14

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: Prerequisite for bug 2029761
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Limited change to text output harness.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/5ff859a2d77f