Bug 1129491
| Summary: | memory corruption after pskc_build_xml() | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | David Woodhouse <dwmw2> | ||||||||
| Component: | oath-toolkit | Assignee: | Jaroslav Škarvada <jskarvad> | ||||||||
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||||
| Severity: | unspecified | Docs Contact: | |||||||||
| Priority: | unspecified | ||||||||||
| Version: | 21 | CC: | dwmw2, jskarvad | ||||||||
| Target Milestone: | --- | ||||||||||
| Target Release: | --- | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Whiteboard: | |||||||||||
| Fixed In Version: | oath-toolkit-2.4.1-9.fc21 | Doc Type: | Bug Fix | ||||||||
| Doc Text: | Story Points: | --- | |||||||||
| Clone Of: | Environment: | ||||||||||
| Last Closed: | 2015-02-09 05:27:08 UTC | Type: | Bug | ||||||||
| Regression: | --- | Mount Type: | --- | ||||||||
| Documentation: | --- | CRM: | |||||||||
| Verified Versions: | Category: | --- | |||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||
| Embargoed: | |||||||||||
| Attachments: |
|
||||||||||
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.
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...
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 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 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). 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. The above test program in attachment 926222 [details] can be licensed under LGPLv2.1+.
Fixed upstream in oath-toolkit-2.6.4: https://gitlab.com/oath-toolkit/oath-toolkit/-/commit/e22ce51078e559c93eddb84e4d7c383892624bc4 |
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== ...