Fix record based files #2511

wcosnc · 2022-02-10T15:59:39Z

wcosnc · 2022-02-11T07:18:11Z

Ups, I have accidentally added some files that were not in .gitignore. What is best practice to remove them from the pull request?

frankmorgner · 2022-02-11T10:22:17Z

Just add a commit with a fixup on-top. if needed, you can cleanup later.

frankmorgner · 2022-02-11T10:25:40Z

force-push is OK, too

frankmorgner · 2022-02-11T12:59:20Z

your changes look a bit rough. please check whether you can split this into multiple commits that are easier to review. Also, you've added comments to the source code regarding the actual change, which instead belongs into the commit message.

…number.

wcosnc · 2022-02-15T10:11:58Z

I have reduced the changes to the known issues and split them into 2 commits. Hope that's better. For the other issues I create appropriate tickets.

wcosnc · 2022-02-17T11:44:38Z

Fuzzing failed. I checked the logs but can't see a relation to the changes. @frankmorgner could you please have a look? Thanks.

Jakuje · 2022-02-17T15:46:09Z

The CIFuzz failed, because some driver tried to allocate some insane memory size, probably because of some change in your code:

==22== ERROR: libFuzzer: out-of-memory (malloc(4217135360))
   To change the out-of-memory limit use -rss_limit_mb=<N>

    #0 0x531741 in __sanitizer_print_stack_trace /src/llvm-project/compiler-rt/lib/asan/asan_stack.cpp:87:3
    #1 0x4715e8 in fuzzer::PrintStackTrace() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtil.cpp:210:5
    #2 0x4559d5 in fuzzer::Fuzzer::HandleMalloc(unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:131:3
    #3 0x4558ea in fuzzer::MallocHook(void const volatile*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:100:6
    #4 0x538d37 in __sanitizer::RunMallocHooks(void const*, unsigned long) /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common.cpp:308:5
    #5 0x4a8b80 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) /src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:611:5
    #6 0x4a84a3 in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) /src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:965:34
    #7 0x52796b in malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:130:10
    #8 0xa2f0b2 in sc_pkcs15emu_din_66291_init_ex /src/opensc/src/libopensc/pkcs15-din-66291.c:224:29
    #9 0x6032c3 in sc_pkcs15_bind_synthetic /src/opensc/src/libopensc/pkcs15-syn.c:133:8
    #10 0x59d1eb in sc_pkcs15_bind /src/opensc/src/libopensc/pkcs15.c:1279:8
    #11 0x560969 in LLVMFuzzerTestOneInput /src/opensc/src/tests/fuzzing/fuzz_pkcs15_reader.c:210:5
    #12 0x457cb3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #13 0x45749a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3
    #14 0x458b69 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:757:19
    #15 0x459835 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:895:5
    #16 0x44904f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6
    #17 0x471da2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #18 0x7f9baa3df0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #19 0x4220dd in _start (build-out/fuzz_pkcs15_reader+0x4220dd)

DEDUP_TOKEN: __sanitizer_print_stack_trace--fuzzer::PrintStackTrace()--fuzzer::Fuzzer::HandleMalloc(unsigned long)
MS: 3 ChangeBit-EraseBytes-ChangeBinInt-; base unit: 2e848338d4ab935913bcc92a25c9ff49a9fcd1a6
0x14,0x0,0x3b,0x7f,0x1,0x0,0x0,0x0,0x6a,0x44,0x4e,0x49,0x65,0x1a,0x0,0x0,0xe0,0xd7,0x0,0x3,0x90,0x0,0x0,0x0,0x2,0x0,0x90,0x2,0x12,0x0,0xc,0x2a,0x80,0x4,0xfb,0x5c,0x61,0x0,0x2,0x0,0x2,0x0,0x61,0x0,0x2,0x0,0x90,0x0,0x0,0x0,0x2,0x0,0x61,0x0,0x2,0x0,0x90,0xff,0x0,0x0,0x2,0x0,0x61,0x0,0x2,0x0,0x61,0x0,0x2,0x0,0x90,0xff,0x0,0x0,0x2,0x0,0x61,0x0,0x2,0x0,0xb0,0x0,
\024\000;\177\001\000\000\000jDNIe\032\000\000\340\327\000\003\220\000\000\000\002\000\220\002\022\000\014*\200\004\373\\a\000\002\000\002\000a\000\002\000\220\000\000\000\002\000a\000\002\000\220\377\000\000\002\000a\000\002\000a\000\002\000\220\377\000\000\002\000a\000\002\000\260\000

I can reproduce it in your branch with:

git clone github.com/google/oss-fuzz && cd oss-fuzz
python3 infra/helper.py build_fuzzers --engine=libfuzzer --sanitizer=address  opensc ~/devel/OpenSC/
echo "FAA7fwEAAABqRE5JZRoAAODXAAOQAAAAAgCQAhIADCqABPtcYQACAAIAYQACAJAAAAACAGEAAgCQ/wAAAgBhAAIAYQACAJD/AAACAGEAAgCwAA==" | base64 -d > /tmp/testcase
python3 infra/helper.py reproduce opensc fuzz_pkcs15_reader /tmp/testcase

This needs to be fixed before we will merge this change. Allocating 4GB of memory for some bugus file that might be on a smart card is wrong. There needs to be some limits. We usually use MAX_FILE_SIZE. If the size is larger, please fail.

Jakuje · 2022-02-17T15:49:37Z

The debug log looks like this if you run with OPENSC_DEBUG=9:

$ python3 infra/helper.py reproduce -e OPENSC_DEBUG=9 opensc fuzz_pkcs15_reader /tmp/testcase
[...]
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:1156:dnie_compose_and_send_apdu: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] apdu.c:550:sc_transmit_apdu: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card.c:473:sc_lock: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card.c:513:sc_lock: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] apdu.c:515:sc_transmit: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] apdu.c:363:sc_single_transmit: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] apdu.c:370:sc_single_transmit: CLA:0, INS:A4, P1:0, P2:0, data(2) 0x7f80e9d088a0
P:8; T:0x140191668266944 15:38:52.504 [fuzz] fuzz_pkcs15_reader.c:83:fuzz_get_chunk: 
Returning fuzzing chunk (18 bytes):
0C 2A 80 04 FB 5C 61 00 02 00 02 00 61 00 02 00 .*...\a.....a...
90 00                                           ..

P:8; T:0x140191668266944 15:38:52.504 [fuzz] apdu.c:382:sc_single_transmit: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] apdu.c:539:sc_transmit: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card.c:523:sc_unlock: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:1777:dnie_check_sw: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:1789:dnie_check_sw: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:1955:dnie_process_fci: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] iso7816.c:363:iso7816_process_fci:   bytes in file: 4217135360
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:2082:dnie_process_fci: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:1195:dnie_compose_and_send_apdu: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card.c:523:sc_unlock: called
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card-dnie.c:1317:dnie_select_file: returning with: 0 (Success)
P:8; T:0x140191668266944 15:38:52.504 [fuzz] card.c:879:sc_select_file: returning with: 0 (Success)

so the problem is in the new handling of process_fci.

frankmorgner · 2022-02-18T07:35:41Z

Well, strictly speaking FB 5C 61 00 is correctly decoded to 4217135360 bytes. However, the given file size should be limited to an upper bound when reading the file. I assume this is a problem in some other places, too...

…ed fuzzing. Also limit record and file size in sc_pkcs15_read_file(). Change result code for record out of bounds.

wcosnc · 2022-02-22T08:32:38Z

AppVeyor build failed with temporary error. Rerun?

Build started
git clone -q https://github.com/OpenSC/OpenSC.git C:\projects\opensc
      1 [main] git-remote-https 591 child_info_fork::abort: \??\C:\cygwin\bin\cygnghttp2-14.dll: Loaded to different address: parent(0x1D0000) != child(0xD00000)
error: cannot fork() for fetch-pack: Resource temporarily unavailable
Command exited with code 128

Jakuje · 2022-02-22T08:43:49Z

src/libopensc/iso7816.c

+						size <<= 8;
+						size |= (uint32_t) p[i];
+					}
+					file->size = (size > MAX_FILE_SIZE)? MAX_FILE_SIZE:size;


I would rather print some debug message if we go over the size (and maybe fail) than just max it on this value.

It is very unlikely that this will happen with real cards, but if it does, verbose log message is great help for debugging.

Also please, keep whitespaces around operators, if, for and similar as in the rest of the code.

The current approach is to check for MAX_FILE_SIZE when the actual allocation is done... Capping this here, is certainly useful, but a debug message would be good.

Jakuje · 2022-02-22T09:04:12Z

src/libopensc/pkcs15.c

+			if(file->record_length > 0) {
+				len = (file->record_length > MAX_FILE_SIZE)? MAX_FILE_SIZE:file->record_length + 5;
+			} else {
+				len = 0x2000 + 5;


is this magic constant coming from somewhere specific or is it just arbitrary number? In any case, it should have some comment in the code so the people reading the code in couple of years do not have to ask the same question.

Jakuje · 2022-02-22T09:13:11Z

src/libopensc/pkcs15.c

+
+				apdu.data = offset_buffer;
+				apdu.datalen = 4;
+				apdu.lc = 4;


maybe sizeof(offset_buffer) would be more readable, but this is probably about personal taste

src/libopensc/pkcs15.c

Jakuje · 2022-02-22T09:19:28Z

src/libopensc/pkcs15.c

+				if (r < 0) goto fail_unlock;
+
+				if(data_do) {
+					memcpy(data + offset_u16, data_do, data_do_len);


I assume you can do this because you added +5 to the data allocated above.

I would consider reading the data into buffer of SC_MAX_APDU_RESP_SIZE size, do the decoding in there and only after that copy the result to the data buffer.

What I think is more important, @wcosnc correctly identified that sc_read_record (or its counterpart read_record in the card driver call backs) doesn't allow specifying an offset. Doing this here, is bad since we want to have the flexibility of implementing this depending on the card driver in use.

Currently, only iso7816.c has an implementation of read_record so changing this would have very minimal impact on the other cards. I suggest you do the following:

add an additional parameter to both sc_read_record and read_record, named idx (just like in sc_read_binary)

Use idx = 0 where sc_read_record is currently called without this parameter

change read_record in iso7816.c so that if and only if idx != 0 your implementation is used with INS=B3. if idx is not specified (i.e. 0), then it should still use the current version with INS=B2

use the new sc_read_record to specify the offset in sc_pkcs15_read_file

Co-authored-by: Jakub Jelen <[email protected]>

frankmorgner · 2022-03-11T13:24:20Z

@wcosnc did you have time to get back to the comments?

wcosnc · 2022-03-11T16:01:23Z

I haven't found the time yet. I'm going to edit the code the next days.

…files

wcosnc · 2022-04-20T14:47:27Z

@frankmorgner Could you please have a look at the tests? I can't see a direct relation to the code. Thank you.

frankmorgner

I've changed the parsing a little bit. (I wanted to make a "suggestion" for a change, but Github pushed the change onto your pull requests, I hope that is OK)

Allowing specification of an offset in sc_read_file is a generic change, that should be fixed properly. Your changes are technically good, but they belong into the lower level of the card abstraction, I think.

frankmorgner · 2022-05-04T15:29:35Z

src/libopensc/pkcs15.c

+				if (r < 0) goto fail_unlock;
+
+				if(data_do) {
+					memcpy(data + offset_u16, data_do, data_do_len);


What I think is more important, @wcosnc correctly identified that sc_read_record (or its counterpart read_record in the card driver call backs) doesn't allow specifying an offset. Doing this here, is bad since we want to have the flexibility of implementing this depending on the card driver in use.

Currently, only iso7816.c has an implementation of read_record so changing this would have very minimal impact on the other cards. I suggest you do the following:

add an additional parameter to both sc_read_record and read_record, named idx (just like in sc_read_binary)

Use idx = 0 where sc_read_record is currently called without this parameter

change read_record in iso7816.c so that if and only if idx != 0 your implementation is used with INS=B3. if idx is not specified (i.e. 0), then it should still use the current version with INS=B2

use the new sc_read_record to specify the offset in sc_pkcs15_read_file

Jakuje · 2022-08-29T12:36:51Z

@wcosnc @frankmorgner what is the status of this PR? Do we want to consider it for 0.23.0? We need at least to resolve conflicts.

frankmorgner · 2023-04-13T13:32:40Z

closing this as #2604 is merged

Normann Cunz added 2 commits February 15, 2022 10:43

Fixes OpenSC#2504 for record based files with index option as record …

5105a06

…number.

Fixes OpenSC#2505. Decode FCI tag as negative file size.

abcb194

wcosnc force-pushed the fix_record_based_files branch from 2aff95d to abcb194 Compare February 15, 2022 09:57

Limit file size in iso7816_process_fci() to MAX_FILE_SIZE due to fail…

1605358

…ed fuzzing. Also limit record and file size in sc_pkcs15_read_file(). Change result code for record out of bounds.

Jakuje reviewed Feb 22, 2022

View reviewed changes

Update src/libopensc/pkcs15.c

fd3987d

Co-authored-by: Jakub Jelen <[email protected]>

Normann Cunz added 3 commits April 3, 2022 16:57

Merge remote-tracking branch 'upstream/master' into fix_record_based_…

abd3c18

…files

Remarks considered. Refers to OpenSC#2504 and OpenSC#2505

6a5a26a

Merge, accept remote

21614ec

use size_t for parsing

96f64aa

frankmorgner requested changes May 4, 2022

View reviewed changes

frankmorgner mentioned this pull request Sep 13, 2022

Fix record based files #2604

Merged

5 tasks

frankmorgner closed this Apr 13, 2023

Fix record based files #2511

Fix record based files #2511

Uh oh!

Conversation

wcosnc commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wcosnc commented Feb 11, 2022

Uh oh!

frankmorgner commented Feb 11, 2022

Uh oh!

frankmorgner commented Feb 11, 2022

Uh oh!

frankmorgner commented Feb 11, 2022

Uh oh!

wcosnc commented Feb 15, 2022

Uh oh!

wcosnc commented Feb 17, 2022

Uh oh!

Jakuje commented Feb 17, 2022

Uh oh!

Jakuje commented Feb 17, 2022

Uh oh!

frankmorgner commented Feb 18, 2022

Uh oh!

wcosnc commented Feb 22, 2022

Uh oh!

Jakuje Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

frankmorgner Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Jakuje Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

Jakuje Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jakuje Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

frankmorgner May 4, 2022

Choose a reason for hiding this comment

Uh oh!

frankmorgner commented Mar 11, 2022

Uh oh!

wcosnc commented Mar 11, 2022

Uh oh!

wcosnc commented Apr 20, 2022

Uh oh!

frankmorgner left a comment

Choose a reason for hiding this comment

Uh oh!

frankmorgner May 4, 2022

Choose a reason for hiding this comment

Uh oh!

Jakuje commented Aug 29, 2022

Uh oh!

frankmorgner commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wcosnc commented Feb 10, 2022 •

edited

Loading