1371890 - out of bounds read in [@ PR_FormatTimeUSEnglish]

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.txt

STR:
1) Serve test_case.txt with ftprequesthandler.py (python2, run -h for details)
2) open the url given by ftprequesthandler.py in Firefox

==71814==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f1ad5a862b8 at pc 0x7f1ad584ec22 bp 0x7ffdf5738dd0 sp 0x7ffdf5738dc8
READ of size 8 at 0x7f1ad5a862b8 thread T0
    #0 0x7f1ad584ec21 in PR_FormatTimeUSEnglish src/nsprpub/pr/src/misc/prtime.c:1866:13
    #1 0x7f1abcb73b39 in nsFTPDirListingConv::DigestBufferLines(char*, nsCString&) src/netwerk/streamconv/converters/nsFTPDirListingConv.cpp:296:9
    #2 0x7f1abcb72477 in nsFTPDirListingConv::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) src/netwerk/streamconv/converters/nsFTPDirListingConv.cpp:131:12
    #3 0x7f1abc5daac4 in nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) src/netwerk/base/nsBaseChannel.cpp:894:28
    #4 0x7f1abc626a59 in nsInputStreamPump::OnStateTransfer() src/netwerk/base/nsInputStreamPump.cpp:619:33
    #5 0x7f1abc625688 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) src/netwerk/base/nsInputStreamPump.cpp:444:25
    #6 0x7f1abc5dbc0f in nsBaseContentStream::DispatchCallback(bool) src/netwerk/base/nsBaseContentStream.cpp:29:13
    #7 0x7f1abcd12caf in DispatchCallbackSync src/netwerk/base/nsBaseContentStream.h:65:33
    #8 0x7f1abcd12caf in nsFtpState::OnInputStreamReady(nsIAsyncInputStream*) src/netwerk/protocol/ftp/nsFtpConnectionThread.cpp:123
    #9 0x7f1abcd12d1c in non-virtual thunk to nsFtpState::OnInputStreamReady(nsIAsyncInputStream*) src/netwerk/protocol/ftp/nsFtpConnectionThread.cpp:116:13
    #10 0x7f1abc41180d in nsInputStreamReadyEvent::Run() src/xpcom/io/nsStreamUtils.cpp:96:20
    #11 0x7f1abc47441e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1321:14
    #12 0x7f1abc480858 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10
    #13 0x7f1abd24cc91 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21
    #14 0x7f1abd1a9d90 in RunInternal src/ipc/chromium/src/base/message_loop.cc:238:10
    #15 0x7f1abd1a9d90 in RunHandler src/ipc/chromium/src/base/message_loop.cc:231
    #16 0x7f1abd1a9d90 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:211
    #17 0x7f1ac26c068f in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
    #18 0x7f1ac5d80ce1 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30
    #19 0x7f1ac5f51334 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22
    #20 0x7f1ac5f52ea0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8
    #21 0x7f1ac5f541f1 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21
    #22 0x4eb5a3 in do_main src/browser/app/nsBrowserApp.cpp:236:22
    #23 0x4eb5a3 in main src/browser/app/nsBrowserApp.cpp:309
    #24 0x7f1ad7e0882f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
    #25 0x41d0f8 in _start (m-c-1496772615-asan-opt/firefox+0x41d0f8)

0x7f1ad5a862b8 is located 8 bytes to the left of global variable 'abbrevMonths' defined in 'src/nsprpub/pr/src/misc/prtime.c:1762:20' (0x7f1ad5a862c0) of size 96
0x7f1ad5a862b8 is located 32 bytes to the right of global variable 'days' defined in 'src/nsprpub/pr/src/misc/prtime.c:1757:20' (0x7f1ad5a86260) of size 56
SUMMARY: AddressSanitizer: global-buffer-overflow src/nsprpub/pr/src/misc/prtime.c:1866:13 in PR_FormatTimeUSEnglish
Shadow bytes around the buggy address:
  0x0fe3dab48c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3dab48c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3dab48c20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3dab48c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3dab48c40: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00 00 00 00
=>0x0fe3dab48c50: 00 00 00 f9 f9 f9 f9[f9]00 00 00 00 00 00 00 00
  0x0fe3dab48c60: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0fe3dab48c70: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0fe3dab48c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3dab48c90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3dab48ca0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

Comment 1

[Tyson Smith [:tsmith]]

Attachment: ftprequesthandler.py

Comment 2

[Gian-Carlo Pascutto [:gcp]]

ParseFTPList doesn't check that the time info it fills into its structure is valid, and PR_FormatTimeUSEnglish assumes it is, leading to an out of bounds read.

Michal, you're both listed as a peer for this module and have touched nearby code recently, so can you take this?

Comment 3

[Michal Novotny [:michal]]

Attachment: fix

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8884589 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 5

[Michal Novotny [:michal]]

(In reply to Michal Novotny (:michal) from comment #4)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

It's easy to crash Firefox. Not sure how easy is to make an useful exploit.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

Yes, I can remove the comment, but the change is so obvious, that the change itself points at the problem.

> Which older supported branches are affected by this flaw?

All

> If not all supported branches, which bug introduced the flaw?
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

This code didn't change for a very long time, so it's likely that the patch will be applicable to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

Low. It's a simple change and I tested it locally.

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> ParseFTPList doesn't check that the time info it fills into its structure is
> valid, and PR_FormatTimeUSEnglish assumes it is,

That assumption isn't documented in the comments for the function or its header. Long ago Debian wontfixed a bug on making the locale-aware equivalent strftime() handle this and I'm sure ours comes from the same era, but we don't use this function much and it's a crazy footgun to leave around. Should we fix NSPR instead? Microsoft has fixed the version in Visual Studio, for example: 
  "if the tm data structure addressed by timeptr is invalid (for example,
  if it contains out of range values for the time or date), or if the format
  string contains an invalid formatting code [...] the function returns 0
  and sets errno to EINVAL."
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-wcsftime-strftime-l-wcsftime-l

I hunted through the other uses of PR_FormatTimeUSEnglish() in our code and all the others look OK though: they use a tm structure created by PR_ExplodeTime().

I don't want to get in the way of patching this security bug, but is it worth cloning a bug to fix NSPR?

Comment 7

[Daniel Veditz [:dveditz]]

(and if so could you do that? if you can't needinfo? back to me so I see this)

Comment 8

[Daniel Veditz [:dveditz]]

Lowering the severity a bit since ADDSTR() makes sure it won't be writing outside the buffer; this leaks a tiny bit of the data segment into the directory listing. I suppose it would be same-origin with an HTML page hosted on the malicious FTP server so it might be able to read back the leaked data but I don't think it would be any user data. Maybe there'd be some padding between the static arrays that would contain left-over garbage from whatever program was loaded before.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 8884589 [details] [diff] [review]
fix

sec-approval=dveditz, but now that I've lowered the severity you no longer need it :-)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aea5888a82edce883da06a66f69521ceac196dc8

Comment 11

[Carsten Book [:Tomcat]]

backed out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=113242600&repo=mozilla-inbound

Comment 12

[Michal Novotny [:michal]]

Attachment: patch v2 - fixed tests

ParseFTPList doesn't fill tm_wday. PR_NormalizeTime fixes it to a correct day of a week, so the tests need to be updated.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a37efd70194b33f644d763cac41442de5a582cd

Comment 14

[Gian-Carlo Pascutto [:gcp]]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> That assumption isn't documented in the comments for the function or its
> header.

I think most standard functions don't explicitly document their resilience in the face of hostile input.

> Long ago Debian wontfixed a bug on making the locale-aware
> equivalent strftime() handle this and I'm sure ours comes from the same era,
> but we don't use this function much and it's a crazy footgun to leave
> around. Should we fix NSPR instead? Microsoft has fixed the version in
> Visual Studio, for example: 
>   "if the tm data structure addressed by timeptr is invalid (for example,
>   if it contains out of range values for the time or date), or if the format
>   string contains an invalid formatting code [...] the function returns 0
>   and sets errno to EINVAL."
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-
> wcsftime-strftime-l-wcsftime-l
> 
> I hunted through the other uses of PR_FormatTimeUSEnglish() in our code and
> all the others look OK though: they use a tm structure created by
> PR_ExplodeTime().
> 
> I don't want to get in the way of patching this security bug, but is it
> worth cloning a bug to fix NSPR?

That seems like a sensible idea, yes.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a37efd70194

need also beta uplift request

Comment 16

[Michal Novotny [:michal]]

Comment on attachment 8885233 [details] [diff] [review]
patch v2 - fixed tests

Approval Request Comment
[Feature/Bug causing the regression]: not a regression
[User impact if declined]: crash on malformed ftp response
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: test case is available in the bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change
[String changes made/needed]: none

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8885233 [details] [diff] [review]
patch v2 - fixed tests

don't crash on parsing ftp dir listing, beta55+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/59ea27873e7b

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Michal Novotny (:michal) from comment #16)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: test case is
> available in the bug

Michal, since this fix has automated coverage, would manual testing actually bring any value here?

Comment 20

[Michal Novotny [:michal]]

The existing tests don't test the crash. So manual testing with the attached testcase can verify, that the crash was fixed.

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Michal Novotny (:michal) from comment #20)
> The existing tests don't test the crash. So manual testing with the attached
> testcase can verify, that the crash was fixed.

Understood. My understanding, based on Comment 16, was quite the opposite.

Flagging for manual testing.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8885233 [details] [diff] [review]
patch v2 - fixed tests

See comment 16.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8885233 [details] [diff] [review]
patch v2 - fixed tests

While this is a sec-mod, the patch and change looks low risk. I discussed this one with Al and he agrees that we could take this in ESR52.3.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7c095249ef8a

Comment 25

[Andrei Vaida [:avaida]]

I was able to reproduce the crash on affected build (asan 55.0a1, 20170609224820) using the instructions from Comment 0 on Ubuntu 16.04 x64.

The crash is no longer occurring on Ubuntu 16.04 x64 using the following builds:

      - asan 52.2esr (20170726030943) 
      - asan 55.0b13 (20170726032849)
      - asan 56.0a1 (20170726020448)