Opensc: Firefox 74.0 Crash

[Counter178]

Firefox crashes with OpenSC 0.20

Firstly I apologize if my bug report isn't very detailed in text-based but I'm forwarding feedback about Firefox Crash with OpenSC 0.20. When asking for more detailed answer the crash report was given:
1st feedback: https://bugzilla.mozilla.org/show_bug.cgi?id=1560486
2nd feedback: https://crash-stats.mozilla.org/report/index/4b860515-e5f4-4fef-b440-29bff0200318#tab-details

Question: In what circumstances does this error happen?

[dengert]

This may have been solved by: https://github.com/OpenSC/OpenSC/issues/1935 and https://github.com/OpenSC/OpenSC/pull/1970

[frankmorgner]

Dustin L. Howett commented:

0:000> k
 # Child-SP          RetAddr           Call Site
00 00000061`1ddfc090 00007fff`d996df9c opensc_pkcs11!_invoke_watson+0x17 [f:\dd\vctools\crt\crtw32\misc\invarg.c @ 132] 
01 00000061`1ddfc0c0 00007fff`d996dfb9 opensc_pkcs11!_invalid_parameter+0x64 [f:\dd\vctools\crt\crtw32\misc\invarg.c @ 85] 
02 00000061`1ddfc100 00007fff`d9972f65 opensc_pkcs11!_invalid_parameter_noinfo+0x19 [f:\dd\vctools\crt\crtw32\misc\invarg.c @ 97] 
03 00000061`1ddfc140 00007fff`d997ed23 opensc_pkcs11!strcpy_s+0x29 [f:\dd\vctools\crt\crtw32\h\tcscpy_s.inl @ 18] 
04 00000061`1ddfc170 00007fff`d983c31c opensc_pkcs11!_strdup+0x3f [f:\dd\vctools\crt\crtw32\string\strdup.c @ 80] 
05 00000061`1ddfc1b0 00007fff`d983c99d opensc_pkcs11!pcsc_add_reader+0x90 [c:\projects\opensc\src\libopensc\reader-pcsc.c @ 1274] 
06 00000061`1ddfc1e0 00007fff`d97aa395 opensc_pkcs11!pcsc_detect_readers+0x27d [c:\projects\opensc\src\libopensc\reader-pcsc.c @ 1403] 
07 00000061`1ddfc270 00007fff`d979a176 opensc_pkcs11!sc_ctx_detect_readers+0x39 [c:\projects\opensc\src\libopensc\ctx.c @ 733] 
08 00000061`1ddfc2a0 00007fff`f86ad296 opensc_pkcs11!C_GetSlotList+0x7e [c:\projects\opensc\src\pkcs11\pkcs11-global.c @ 407] 
09 00000061`1ddfc2f0 00007fff`e12e7434 nss3!SECMOD_UpdateSlotList+0x66 [/builds/worker/checkouts/gecko/security/nss/lib/pk11wrap/pk11util.c @ 1028] 
0a 00000061`1ddfc360 00007fff`e12f0351 xul!nsNSSComponent::CheckForSmartCardChanges+0xe4 [/builds/worker/checkouts/gecko/security/manager/ssl/nsNSSComponent.cpp @ 849] 
0b (Inline Function) --------`-------- xul!CheckForSmartCardChanges+0x3b [/builds/worker/checkouts/gecko/security/manager/ssl/nsNSSComponent.h @ 149] 
0c 00000061`1ddfc3e0 00007fff`e29313b2 xul!nsPKCS11Module::ListSlots+0x61 [/builds/worker/checkouts/gecko/security/manager/ssl/nsPKCS11Slot.cpp @ 250] 
0d 00000061`1ddfc470 00007fff`ddccf573 xul!XPTC__InvokebyIndex+0x72

from pcsc_add_reader:

0:000> dv
            ctx = 0x000001a2`cefff5d0
    reader_name = 0x00000061`1ddfc228 " \i???"
reader_name_len = 0x000001a2`c680a74f
     out_reader = 0x000001a2`cedd1840

reader_name and reader_name_len appear to be bogus. This is likely an upstream issue that Firefox can't necessarily fix.

[frankmorgner]

We could add this length check:

diff --git a/src/libopensc/reader-pcsc.c b/src/libopensc/reader-pcsc.c
index 34dc821b..05430a66 100644
--- a/src/libopensc/reader-pcsc.c
+++ b/src/libopensc/reader-pcsc.c
@@ -1473,7 +1473,8 @@ static int pcsc_detect_readers(sc_context_t *ctx)
                        goto out;
                }

-               for (reader_name = reader_buf; *reader_name != '\x0';
+               for (reader_name = reader_buf;
+                               *reader_name != '\x0' && (DWORD)(reader_name - reader_buf) < reader_buf_size;
                                reader_name += strlen(reader_name) + 1) {
                        if (!strcmp(reader->name, reader_name)) {
                                if (reader->flags & SC_READER_REMOVED) {
@@ -1485,7 +1486,7 @@ static int pcsc_detect_readers(sc_context_t *ctx)
                        }
                }

-               if (*reader_name != '\x0') {
+               if (*reader_name != '\x0' && (DWORD)(reader_name - reader_buf) < reader_buf_size) {
                        /* existing reader found; remove it from the list */
                        char *next_reader_name = reader_name + strlen(reader_name) + 1;

@@ -1502,7 +1503,8 @@ static int pcsc_detect_readers(sc_context_t *ctx)
        }

        /* add readers remaining in the list */
-       for (reader_name = reader_buf; *reader_name != '\x0';
+       for (reader_name = reader_buf;
+                       *reader_name != '\x0' && (DWORD)(reader_name - reader_buf) < reader_buf_size;
                        reader_name += strlen(reader_name) + 1) {
                sc_reader_t *reader = NULL;
                struct pcsc_private_data *priv = NULL;

... but I feel there's something else missing to get to the root of the problem.

[Counter178]

"but I feel there's something else missing to get to the root of the problem."
Anything else I can dig-up more? I assume swapping readers doesn't resolve a thing?

[dmmedia]

Hello, I am having the same issue with Firefox 75.0. It just crashes intermittently without starting Mozilla Crash Reporter. Only bad_module_info has stopped working error appears and has a button to close the program. Clicking it, closes Firefox. After uninstalling OpenSC 0.20 which was installed with Estonian ID card software, crashes stopped. Please request more info if needed.

[Counter178]

One thing, that I got from older logs, was the OpenSC log with the
4G modem. Any kind of idea if those problems are related?
80923_ML-opensc-debug_20200219.log.txt

Though last time the problem was fixed: https://github.com/OpenSC/OpenSC/issues/1847 and I haven't gotten any response from the issuer yet.

[frankmorgner]

I think it's clear, that we are reading beyond the list of readers. But I don't understand why this happens. We're checking for the double NUL termination, which a well behaved PC/SC implementation should have...

Can you reliably reproduce or debug the problem?

For a simple debug message, you could add the following patch to the log the output data:

diff --git a/src/libopensc/reader-pcsc.c b/src/libopensc/reader-pcsc.c
index 34dc821bf..583903f17 100644
--- a/src/libopensc/reader-pcsc.c
+++ b/src/libopensc/reader-pcsc.c
@@ -1458,6 +1458,8 @@ static int pcsc_detect_readers(sc_context_t *ctx)
        }
        rv = gpriv->SCardListReaders(gpriv->pcsc_ctx, mszGroups, reader_buf,
                        (LPDWORD) &reader_buf_size);
+       sc_log(ctx, "SCardListReaders (%d bytes):\n%s", reader_buf_size, sc_dump_hex((unsigned char *) reader_buf, reader_buf_size));
+
        if (rv != SCARD_S_SUCCESS) {
                PCSC_LOG(ctx, "SCardListReaders failed", rv);
                ret = pcsc_to_opensc_error(rv);

[matstammisto]

I can reproduce it fairly easily on Firefox 75.0.
Steps:

1. install eID software
2. do random browsing
3. get a crash upon opening a new tab

It won't happen every time, but often enough to make the software unusable. These crashes take place regardless of having a smart card reader connected and the card plugged in, since I almost enver use it have experienced this crashing only with no readers connected.

[gabrielesvelto]

Chiming in because I work on Firefox stability and I've already talked with multiple users experiencing crashes with the latest stable version of OpenSC installed. As a band-aid the next version of Firefox (currently in beta) will block the injection of this version of OpenSC but I'd rather see the issue fixed upstream than degrade functionality to prevent crashes.

I have access to multiple Windows crash dumps where this issue manifests itself and I can pull out stack traces and other relevant information for you. Just let me know what's needed to solve it.

[frankmorgner]

Do you know what the value of reader_buf, reader_buf_size in pcsc_detect_readers is at the time of the crash?

Where did you download your version of OpenSC? It appears that your not using https://github.com/OpenSC/OpenSC/releases/download/0.20.0/OpenSC-0.20.0_win64.msi

[frankmorgner]

I've pushed the suggested changes from above. You can find an installer in the AppVeyor's build artifacts Could you create a debug log with that installer?

[dengert]

Just a thought: https://docs.microsoft.com/en-us/windows/win32/api/winscard/nf-winscard-scardlistreadersa
shows in example:

      // Advance to the next value.
            pReader = pReader + wcslen((wchar_t *)pReader) + 1;

Note the wcslen even though this is in the SCardListReadersA section.

[dengert]

On my windows 10 pro with Firefox 75.0, and OpenSC from pr1987 built on master from yesterday, no problems. Have had two devices PIV and SC-HSM plugged in and removed and reinserted in many combinations.

BUT I had a version of OpenSC from 3/8/2020 installed before, and with both devices plugged in then start FireFox, it would start reading from the cards then would close. It produced a 5GB opensc-debug.txt file, built from pr1975. 60MB from todays testing. it looks like OpenSC i doing the global locking in this run.
(PR1975 was designed to show if locking was enabled. and add the VSS traces)

Not clear if reader name is an issue today. We may have it fixed in last month or so?

[metsma]

Do you know what the value of reader_buf, reader_buf_size in pcsc_detect_readers is at the time of the crash?

Where did you download your version of OpenSC? It appears that your not using https://github.com/OpenSC/OpenSC/releases/download/0.20.0/OpenSC-0.20.0_win64.msi

It is the release binaries distributed in http://installer.id.ee/. Diffrerence is that the dll is signed

[martinpaljak]

A small remark, just that everybody would be aware of it:

Due to the change in Firefox (https://github.com/mozilla/gecko-dev/commit/4d14cf5024c65ab3b9d3c20fe80531244d0d25ad), where the reason for the exception is only triggered by installations of the Estonian eID software, loading a module with the given filename is not possible with FF76, even if the module comes from a different source/build and just shares the name (like the unsigned upstream release).

I believe the number of Windows installations of OpenSC via this delivery method is pretty huge (in the ballpark of 600k+ installs, last time I knew), and this is more likely unwanted collateral damage or maybe even "statistical bias". Unfortunate, and I hope @open-eid team will allocate sufficient resources to get this sorted out ASAP and the fix pushed upstream.

Is there anyone on this thread who have a setup where this issue replicates reliably, so that the environment could be analyzed or maybe even debugging be performed? /asking for a friend/

[dmmedia]

@martinpaljak I have FF 75.0 with pending update to 76.0 with reproducable issue, also I am SW developer and understand how things work. However I have no time at the moment to find out myself how to setup all the debugging environment for FF, but I can follow the guide in the meanwhile and setup/run/debug anything relevant. If you won't find anyone with a bit more spare time, I'd be glad to help within these limits.

[dengert]

@dmmedia One thing you could do is install OpenSC nightly build to see if the problem is fixed already in current source. See: https://github.com/OpenSC/Nightly/
https://github.com/OpenSC/Nightly/branches
I believe it may have been fixed by #1975 turning on PKCS11_THREAD_LOCKING in Windows.

[Counter178]

Debugging through this might be able to show something:
@matstammisto Could You also debug with these instructions?

1. Install ID-card software from installer.id.ee
2. Install OpenSC https://ci.appveyor.com/project/metsma/opensc/builds/32708408/job/h2u1gs1qf72xvyie/artifacts
3. Open Firefox Security Devices and remove PKCS11 module which was installed with ID software.
4. Disable extension PKCS11 loader
5. Open OpenSC Conf file C:\program files\OpenSC Project\OpenSC\opensc.conf and add:
   debug = 9;
   debug_file = %TEMP%\opensc-debug.txt; but replace %TEMP% with its actual value.
6. Try repeating the Firefox crash and save the OpenSC log.

[dengert]

@metsma could you rebase your repro https://github.com/metsma/OpenSC which says: "This branch is 54 commits behind OpenSC:master" and rebuild new version of appveyor before @Counter178 and @matstammisto does more testing. This may have been solved.

[metsma]

The build is from https://github.com/metsma/OpenSC/tree/build-0.20.1.3

[frankmorgner]

Above, you wrote that the binary in question is a signed dll of 0.20.0. The released you've just referenced, however, has a ton of changes compared with the 0.20.0 release... Do you have time to analyze the problem?

[frankmorgner]

By the way, is there any support contact that ID-card software users should go to in case of problems?

[frankmorgner]

Ideally, I'd like to hear back from someone who can reproduce the problem. As mentioned above, https://ci.appveyor.com/project/metsma/opensc/builds/32708408/job/h2u1gs1qf72xvyie/artifacts contains both, most up to date fixes to known problems and some additional debugging of the problem which causes the crash...

[matstammisto]

By the way, is there any support contact that _ID-card software_ users should go to in case of problems?

Yes there is, I've reported it to them and they are aware of this thread. I'll follow the instructions above to try and reproduce after office hours.

[Counter178]

Ideally, I'd like to hear back from someone who can reproduce the problem. As mentioned above, https://ci.appveyor.com/project/metsma/opensc/builds/32708408/job/h2u1gs1qf72xvyie/artifacts contains both, most up to date fixes to known problems and some additional debugging of the problem which causes the crash...

Concerning ID-card software, known users who reported with OpenSC and Firefox crash we're given the same instructions to make debug logs. If there are more users, who can reproduce the problem, the necessary info would be put in here.

[metsma]

Looking one of the crashdump. Seems like there is input from nss already invalid? also context is invalid memory.

[Jakuje]

ping. Is there any update on this or anything I can help with? This looks like last thing we should fix before going into the release or 0.21.0 (if it is not already fixed in master). Would it be possible to make 0.21.0-rc1 to kick off some testing?

[Counter178]

If anyone can reproduce the firefox crash(vers. 77) issue with the latest eID software 20.05, then I would recommend contacting software developer as well at [email protected] and send logs.

[misterzed88]

Thanks to the good, solid info from everyone involved in this issue, I was able to analyze its root causes and is working on a fix. The issue was due to a combination of OpenSC and Windows problems:

• OpenSC: In pcsc_detect_readers, when SCardEstablishContext fails for some reason, the gpriv->pcsc_ctx is set to NULL as a side effect. This context is used in subsequent calls without any renewed attempts with SCardEstablishContext.
• Windows: When SCardListReaders is called with a NULL context (which is allowed by the API), and the reader list is empty in the registry, the function returns without zero-terminating the buffer.

You can do the following to trigger the problem:

1. Stop the Smart Card service in Windows, to force the failure in SCardEstablishContext.
2. Remove all subkeys in HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Cryptography\Calais\Readers, to force the bug in SCardListReaders.

Note that the SCardEstablishContext failure can happen intermittently in newer Windows 10 versions even when the Smart Card service is running. This may be the reason for the bug popping up recently. See:
https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/de9670ab-3971-4a2e-a1b1-51bdafbd667d/why-does-scardestablishcontext-return-scardenoservice-even-though-scardsvr-is-running?forum=windowsgeneraldevelopmentissues

[misterzed88]

The example program below shows the problem in the Windows implementation of the SCardListReaders API. When run with an empty reader list in the registry, it reports "Missing zero terminator in reader list". The bug is present from Windows 7 at least, and only in the ANSI variant (SCardListReadersA). The Unicode version works fine.

I plan to report this to Microsoft when I find a proper communication channel for it.

#include <windows.h>
#include <winscard.h>
#include <stdio.h>
#include <stdlib.h>

#pragma comment(lib, "winscard.lib")

int main(void)
{
    char *buf;
    DWORD bufSize = 0;
    char *p;
    DWORD r;

    if ((r = SCardListReadersA(0, NULL, NULL, &bufSize)) != SCARD_S_SUCCESS) {
        printf("Failed to get reader list size: %08x\n", r);
        return 1;
    }

    if (!bufSize) {
        printf("Invalid reader list size\n");
        return 1;
    }

    buf = malloc(bufSize);
    memset(buf, 0xff, bufSize);

    if ((r = SCardListReadersA(0, NULL, buf, &bufSize)) != SCARD_S_SUCCESS) {
        printf("Failed to get reader list: %08x\n", r);
        return 1;
    }

    if (buf[bufSize - 1]) {
        printf("Missing zero terminator in reader list\n");
        return 1;
    }

    if (!buf[0]) {
        printf("No readers found\n");
        return 0;
    }

    printf("Reader list:\n");
    for (p = buf; *p != '\0'; p += strlen(p) + 1)
        printf("%s\n", p);

    return 0;
}

[metsma]

• OpenSC: In pcsc_detect_readers, when SCardEstablishContext fails for some reason, the gpriv->pcsc_ctx is set to NULL as a side effect. This context is used in subsequent calls without any renewed attempts with SCardEstablishContext.

Actually this is expected behavior since Windows 8 (I need verify version). If no SmartCard reader is never attached (clean install) to system the SCardService is default disabled

[frankmorgner]

Do we need to check for the returned buffer size instead of the NUL termination, as suggested above?

[misterzed88]

I would say that there is no need for it, since the NUL termination is the correct response according to the API, and changing the code in that way would just make it more confusing.

Since we never call SCardListReadersA with a NULL context, we avoid the bug with invalid data. But if we still did get non-terminated data back for some reason then the fix suggested above would still fail since strlen may read outside of the buffer bounds when there is a missing zero terminator, which in theory could result in a page access violation if reaching outside of allocated memory.

If you prefer us to be paranoid, what would you think about this fix instead?

--- a/src/libopensc/reader-pcsc.c
+++ b/src/libopensc/reader-pcsc.c
@@ -1448,7 +1448,8 @@ static int pcsc_detect_readers(sc_context_t *ctx)
                }
        } while (rv != SCARD_S_SUCCESS);

-       reader_buf = malloc(sizeof(char) * reader_buf_size);
+       /* The +2 below is to make sure we have zero terminators, in case we get invalid data */
+       reader_buf = calloc(reader_buf_size+2, sizeof(char));
        if (!reader_buf) {
                ret = SC_ERROR_OUT_OF_MEMORY;
                goto out;

[dengert]

And then also check the results of strlen to see if it must have used one or 2 of the extra bytes?

Are you also going to set reader_buf_size +=2; before calling SCardListReadersA to match the actual size of allocated buffer. (Could also allocate +4 bytes, and pass in reader_buf_size +=2;

The problem could be the Microsoft code never added the 1 or 2 bytes and it appeared to work when following 2 bytes happen to be zero. Using calloc with +2 or +4 would guarantee the bytes are zero. A message in opensc-debug.log if one or two of the extra bytes where actually used.

The problem with SCardListReadersA vs SCardListReadersW which you said always worked, must be different code paths, as the string terminator is one vs two null bytes. The bug could be if the list fit exactly in reader_buf_size the final terminator is not added. Could the routine expects the the caller will only look at the reader_buf_size.

[misterzed88]

Just to clarify to avoid any confusion: the SCardListReaders API definition is crystal clear:

• The returned size always includes the size for all zero terminators.
• The returned data is always in the MULTI_SZ format, with zero terminators between strings and a final zero terminator for the end of the list.

The pcsc_detect_readers code is therefore correct to take the size returned from the first call and provide it on the second call. And the code is also correct in trusting the API to return valid data according to the API.

Now for the bug in the Microsoft implementation: it is rather exotic since normally no one calls SCardListReaders with a NULL context (and NULL is BTW not accepted in pcsc-lite). Avoiding such calls is therefore the main part of the proposed fix. But if we want to prepare for the unprobable case that the API is broken in some, new, unexpected way, we should do this with the minimum amount of code and complexity. Adding two sentinel zero bytes gracefully handles the unlikely case that we get unterminated data back (the first one for end of string and the second one for end of list).

Regarding the internal details of the Microsoft bug: as you hinted, the implementation internally reads the names in Unicode and the ANSI version of SCardListReaders converts the strings to ANSI. This implementation obviously "forgot" to add the final list terminator when the reader name list is empty. This has nothing to do with the caller providing too small buffer size for the terminator to fit, because in that case SCardListReaders would return an error. My tests actually show that the buffer size is large enough: with an empty reader list, the first SCardListReaders call returns a size of 1 character (which is the space for the list terminator), which is accepted as a valid size on the second call. But the second call leaves the 1-byte buffer unchanged, when it should have added the zero list terminator in it.

[dengert]

OK, "SCardListReaders API definition is crystal clear" but their code is not. If the only failure is "since normally no one calls SCardListReaders with a NULL context" and you have a fix. Great.

You proposal looks good but its not clear when Microsoft would fix it, or there are other thing we could do.

But from what I have read, there were reports that the reader list changed size. So any assistance is avoiding this problem by the application would be good. The two extra bytes might be enough. For example using a much larger buffer than required or alway using calloc.

Or in the case you site, the first call returns 1, just enough for the terminator, so the second call could be skipped.

The last thing OpenSC needs is to be blacklisted by Mozilla.

[frankmorgner]

Could you check if the problem is fixed with 1dc359c?