Comment 9 for bug 1781529

Revision history for this message
Seth Arnold (seth-arnold) wrote :

Hello, I reviewed mecab version 0.996-6 as checked into cosmic. This is
not a full security audit but rather a quick gauge of maintainability.

- One CVE for mecab in our CVE database.
- mecab is a japanese natural language parser

- Build-Depends: debhelper
- Does not daemonize
- Does not do networking
- postinst looks autogenerated
- No initscript
- No systemd unit files
- No dbus services
- No setuid
- mecab-config and mecab binaries in path
- No sudo fragments
- No udev rules
- Some tests run during the build, I didn't investigate their depth
- No cronjobs
- Relatively clean build logs

- No subprocesses spawned
- Memory management is way too manual; I didn't spot any errors but this
  code would benefit from a C++14-aware rewrite.
- Logging looked good
- Environment variables HOME and MECABRC used, looked fine
- No privileged operations
- No cryptography
- No networking
- No privileged portions of code
- No temporary files
- No WebKit
- No JavaScript
- No PolicyKit

Here's some issues I found while reading the code; these may or may not
have security relevance. The misleading error messages are just going to
be annoying for users.

- cppcheck reports uninitialized variable:
  [src/darts.h:117]: (error) Uninitialized struct member: tmp_node.right

- StringBuffer::reserve() doesn't appear to handle irresponsible length
  increases -- it moves the security boundary out to all callers of this
  routine, including StringBuffer::write(const char* str, size_t length)

- dtoa() in ./src/utils.h can be made to overflow the 64 byte buffer
  provided by _DTOA() if called with DBL_MAX or potentially other inputs.
  This is then exposed via StringBuffer operator<<().

- Iconv::convert() in ./src/iconv_utils.cpp looks vulnerable to an integer
  overflow if given a too-long str parameter

- copy() in ./src/dictionary_generator.cpp has a misleading error message
  "permission denied" that may not reflect the actual error.

- genmatrix() in ./src/dictionary_generator.cpp has a misleading error
  message "permission denied" that may not reflect the actual error.

- compile() in ./src/dictionary.cpp has a misleading error message
  "permission denied" that may not reflect the actual error.

I couldn't actually tell what the code *does* but it appears to do a good
job of checking calls for errors, checking inputs where that makes sense,
etc. As much as I'd love to see this moved to a C++-14 style, there's
something to be said for code that also appears to be pretty static.
(CVE-2007-3231 was fixed in mecab version 0.96. Maybe mecab is *too*
static, the webpage I found suggests no new changes since 2013.)

Security team ACK for promoting mecab to main.

Thanks