← Back to branch summary

~ubuntu-security/ubuntu-cve-tracker/master

« back to all changes in this revision

Viewing changes to mir_reviews/xml-security-c/audits/bug.txt

  • Committer: Steve Beattie
  • Date: 2019-02-19 06:18:27 UTC
  • Revision ID: sbeattie@ubuntu.com-20190219061827-oh57fzcfc1u9dlfk
The ubuntu-cve-tracker project has been converted to git.

Please use 'git clone https://git.launchpad.net/ubuntu-cve-tracker' to
get the converted tree.

Show diffs side-by-side

added added

removed removed

Lines of Context:
mir_reviews/xml-security-c/audits/bug.txt
1
 
Hello, I reviewed xml-security-c 1.7.3-1 as checked into Ubuntu Xenial
2
 
as part of the request to add xml-security-c to Ubuntu main. While this
3
 
shouldn't be considered a security audit, rather just a quick gauge
4
 
of maintainability, I found enough issues that may be exploitable and
5
 
other issues that represent significant risks that I wanted to contact
6
 
the Apache security team directly.
7
 
 
8
 
Here's the notes I've collected.
9
 
 
10
 
safeBuffer feels extremely confused -- a "buffer type" is set via
11
 
non-mandatory field which allows possibilities of type confusion attacks
12
 
sometimes set after use, and no clear method naming exists to indicate
13
 
which is which. Maybe the different types should have different classes
14
 
to prevent type confusion. The 'string' class should probably use
15
 
std::string, because these hand-rolled functions look like significant
16
 
risks.
17
 
 
18
 
The hackish "doubles size" in checkAndExpand() is the only thing keeping
19
 
the rest of this file from being trivially exploitable. It's fairly sloppy
20
 
code. I'd hate to rely upon allocating twice as memory as needed in order
21
 
to avoid the usual raft of memory handling errors.
22
 
 
23
 
- safeBuffer::checkAndExpand() doubles size without checking for integer
24
 
  overflow.
25
 
- safeBuffer::checkAndExpand() doubles size -- which may be a signed
26
 
  'long' type if xerces-c's autoconf generates that configuration -- and
27
 
  thus may invoke undefined behaviour.
28
 
- safeBuffer::sbStrncpyIn(const char * inStr, xsecsize_t n) fails to
29
 
  NUL terminate the buffer if n is less than strlen(inStr);
30
 
- safeBuffer::sbStrncatIn(const char * inStr, xsecsize_t n) fails to NUL
31
 
  terminate the buffer if n < strlen(inStr)
32
 
- safeBuffer::sbMemcpyOut() doesn't validate that 'n' is valid for buffer.
33
 
- safeBuffer::sbStrinsIn(const char * inStr, xsecsize_t offset) doesn't
34
 
  validate that offset + il + (bl - offset + 1) is within buffer.
35
 
- safeBuffer::safeBuffer(xsecsize_t initialSize) does not initialize
36
 
  m_bufferType
37
 
- safeBuffer::sbMemshift(xsecsize_t toOffset, xsecsize_t fromOffset,
38
 
  xsecsize_t len) does not validate that the len is valid for the buffer,
39
 
  how does it handle shrinking? growing? How is this thing useful?
40
 
- safeBuffer::sbStrcatIn(const char * inStr) does not check that
41
 
  strlen(buffer) + strlen(inStr) doesn't overflow
42
 
- safeBuffer::sbStrcatIn(const safeBuffer & inStr) does not check that
43
 
  strlen((char *) buffer) + strlen((char *) inStr.buffer) + 2 doesn't
44
 
  overflow
45
 
 
46
 
The hand-rolled HTTP 1.0-ish implementation looks very primitive and
47
 
probably should be replaced entirely with a good HTTP client library. (At
48
 
least, if supporting HTTP queries in this library is even a good idea --
49
 
I wouldn't expect or want a signature check to potentially read files
50
 
off the filesystem or remote servers.)
51
 
 
52
 
- XSECBinHTTPURIInputStream allocates char fBuffer[4000].
53
 
  XSECBinHTTPURIInputStream::getSocketHandle() writes a path, query,
54
 
  fragment, and hostname into it without checking for overflow.
55
 
- XSECBinHTTPURIInputStream::getSocketHandle() may copy data from beyond
56
 
  the end of fBuffer into redirectBuf[].
57
 
 
58
 
- xlatHexDigit() does not error on unexpected inputs
59
 
 
60
 
- loadX509() leaks 'f'
61
 
 
62
 
- winutils/XSECSOAPRequestorSimpleWin32.cpp
63
 
  unixutils/XSECBinHTTPURIInputStream.cpp
64
 
  use inet_addr() rather than getaddrinfo(); this seems unlikely to
65
 
  support ipv6 addresses well.
66
 
 
67
 
 
68
 
I've also been running American Fuzzy Lop against the xsec-checksig tool.
69
 
I think all it's found so far are malformed URL exceptions. While these
70
 
may not be real issues they complicate fuzzing and I wouldn't expect
71
 
a signature verification tool to abort for something as minor as a
72
 
malformed URL.
73
 
 
74
 
I'll send along a set of crashers eventually; if all the fuzzers ever find
75
 
is malformed URL exceptions, I won't bother you with them.
76
 
 
77
 
Thanks