Bug 1129491 - memory corruption after pskc_build_xml()
Summary: memory corruption after pskc_build_xml()
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: oath-toolkit
Version: 21
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jaroslav Škarvada
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-08-12 22:12 UTC by David Woodhouse
Modified: 2021-05-17 21:26 UTC (History)
2 users (show)

Fixed In Version: oath-toolkit-2.4.1-9.fc21
Clone Of:
Environment:
Last Closed: 2015-02-09 05:27:08 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
test program (1.30 KB, text/x-c++src)
2014-08-12 22:12 UTC, David Woodhouse
no flags Details
hack (2.34 KB, patch)
2014-08-12 22:57 UTC, David Woodhouse
no flags Details | Diff
less horrid, but less efficient fix (1.44 KB, patch)
2014-08-12 23:02 UTC, David Woodhouse
no flags Details | Diff

Description David Woodhouse 2014-08-12 22:12:23 UTC
Created attachment 926222 [details]
test program

Build attached test case with 
gcc -g -o pskctest pskctest.c $(pkg-config --cflags --libs libpskc)

It loads a HOTP key, bumps the counter, and then attempts to save it. Twice.

In pskc_build_xml() we build a new xmlDoc and free the old one... and it looks like we still have references into the old one.

==9202== Memcheck, a memory error detector
==9202== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==9202== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==9202== Command: ./pskctest
==9202== 
Counter: 35
output: <?xml version="1.0" encoding="UTF-8"?>
<KeyContainer xmlns="urn:ietf:params:xml:ns:keyprov:pskc" Version="1.0" Id="exampleID1"><KeyPackage><Key Id="12345678" Algorithm="urn:ietf:params:xml:ns:keyprov:pskc:hotp"><Data><Secret><PlainValue>YoTOTvAMztz6JhLQGaoUqhS1f/C0+Jr0B7xIfA0=</PlainValue></Secret><Counter><PlainValue>35</PlainValue></Counter></Data></Key></KeyPackage></KeyContainer>

New XML: '<?xml version="1.0"?>
<KeyContainer xmlns="urn:ietf:params:xml:ns:keyprov:pskc" Version="1.0" Id="exampleID1"><KeyPackage><Key Id="12345678" Algorithm="urn:ietf:params:xml:ns:keyprov:pskc:hotp"><Data><Secret><PlainValue>YoTOTvAMztz6JhLQGaoUqhS1f/C0+Jr0B7xIfA0=</PlainValue></Secret><Counter><PlainValue>36</PlainValue></Counter></Data></Key></KeyPackage></KeyContainer>
'
==9202== Invalid read of size 1
==9202==    at 0x30554AF0C3: xmlCheckUTF8 (xmlstring.c:788)
==9202==    by 0x305545638F: xmlNewPropInternal (tree.c:1869)
==9202==    by 0x372E80713B: pskc_build_xml (build.c:444)
==9202==    by 0x400BA3: main (pskctest.c:42)
==9202==  Address 0x4cb6ee3 is 147 bytes inside a block of size 1,048 free'd
==9202==    at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9202==    by 0x30554FF679: xmlDictFree (dict.c:809)
==9202==    by 0x372E807BB3: pskc_build_xml (build.c:514)
==9202==    by 0x400B72: main (pskctest.c:40)
==9202== 
==9202== Invalid read of size 1
==9202==    at 0x30554AF0D3: xmlCheckUTF8 (xmlstring.c:788)
==9202==    by 0x305545638F: xmlNewPropInternal (tree.c:1869)
==9202==    by 0x372E80713B: pskc_build_xml (build.c:444)
==9202==    by 0x400BA3: main (pskctest.c:42)
==9202==  Address 0x4cb6ee4 is 148 bytes inside a block of size 1,048 free'd
==9202==    at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9202==    by 0x30554FF679: xmlDictFree (dict.c:809)
==9202==    by 0x372E807BB3: pskc_build_xml (build.c:514)
==9202==    by 0x400B72: main (pskctest.c:40)
==9202== 
...

Comment 1 David Woodhouse 2014-08-12 22:57:47 UTC
Created attachment 926224 [details]
hack

This horrid proof-of-concept hack kind of fixes it for me. It'll obviously fail if there's more than one key in the file, because I deliberately didn't let it reallocate the keypackages array (since the user may have used pskc_get_keypackage() and expect that pointer to continue working).

It probably also fails in other ways too, but it demonstrates the cause of the issue quite clearly.

Other approaches we could take might be to change the internal pointers as we build up the new XML document, or perhaps just to keep a copy of the *original* XML document from which the pskc_t was parsed, and not free that one.

Comment 2 David Woodhouse 2014-08-12 23:02:49 UTC
Created attachment 926225 [details]
less horrid, but less efficient fix

Actually, retaining the original xmldoc seems like a nice and simple approach. Although it's not as efficient as fixing all the pointers up...

Comment 3 Jaroslav Škarvada 2015-01-30 13:41:40 UTC
Thanks for the patch. I think the inefficiency shouldn't be problem here and I like it's simple. Applying it in Fedora and forwarded upstream:
https://savannah.nongnu.org/support/index.php?108736

Comment 4 Fedora Update System 2015-01-30 15:37:41 UTC
oath-toolkit-2.4.1-9.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/oath-toolkit-2.4.1-9.fc21

Comment 5 Fedora Update System 2015-02-01 00:24:41 UTC
Package oath-toolkit-2.4.1-9.fc21:
* should fix your issue,
* was pushed to the Fedora 21 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing oath-toolkit-2.4.1-9.fc21'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2015-1484/oath-toolkit-2.4.1-9.fc21
then log in and leave karma (feedback).

Comment 6 Fedora Update System 2015-02-09 05:27:08 UTC
oath-toolkit-2.4.1-9.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 7 David Woodhouse 2020-11-11 17:25:34 UTC
The above test program in attachment 926222 [details] can be licensed under LGPLv2.1+.

Comment 8 David Woodhouse 2021-05-17 21:26:07 UTC
Fixed upstream in oath-toolkit-2.6.4: https://gitlab.com/oath-toolkit/oath-toolkit/-/commit/e22ce51078e559c93eddb84e4d7c383892624bc4


Note You need to log in before you can comment on or make changes to this bug.