Comment 22 for bug 1891682

Revision history for this message
Alex Murray (alexmurray) wrote :

I reviewed sane-airscan 0.99.24-1 as checked into hirsute. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

sane-airscan is a package that provides a sane backend which supports
driverless network-connected scanners.

- CVE History:
  - No CVE history, this is a relatively new package that was only first
    released in December 2019 and only exists in Ubuntu since groovy.
- Build-Depends
  - gnutls, libavahi, libxml2 and libjpeg-turbo are all depedencies
- No pre/post inst/rm scripts
- No init scripts
- No systemd units
- No dbus services
- No setuid binaries
- 1 binary in PATH
  - -rwxr-xr-x root/root 175312 2021-02-08 01:49 ./usr/bin/airscan-discover
- No sudo fragments
- No polkit files
- No udev rules
- unit tests / autopkgtests
  - unit tests are compiled in the build log but do not appear to be run
    during the build - these appear to just do simple testing of image
    decoding, URI parsing and HTTP multipart decoding. There are also tests
    for the zeroconf handling and these appear to be more extensive. These
    should be executed during the build itself.
  - there are no autopkgtests
- No cron jobs
- Build logs are quite clean

- No processes spawned
- Memory management
  - sane-airscan is all in C so there is a fair amount of dynamic memory
    management - this all appears to be done quite carefully, Coverity only
    found a couple issues (detailed below) but in general this is quite
    good quality code.
- File IO
  - Loads inifile from /etc/sane.d or from path specified in environment
    variable CONFIG_PATH_ENV - this appears to be quite defensive and safe
- Extensive logging but this all appears safe
- Environment variable usage:
  - CONFIG_PATH_ENV - as noted above for the path to the configuration directory
  - HOME - use of this is safely, checking length etc before use - this is
    used to be able to expand ~ in configuration settings to $HOME etc
  - CONFIG_ENV_AIRSCAN_DEBUG - this is used to be able to turn on debug
    logging
- No use of privileged functions
- Use of cryptography / random number sources
  - Uses /dev/urandom to generate random value for retransmit timer - this
    is not security sensitive but /dev/urandom is a good choice since this
    is non-blocking
  - Uses gnutls for SHA256 hash and TLS for HTTPS - but doesn't perform any
    certificate verification of the remote host - but this is expected
    since scanners on a local network are not going to have FQDN nor
    appropriately issued certificates (ie they are likely to just be
    self-signed)
- No use of temp files
- Use of networking is generally defensive but relies on dependent
  libraries to do parsing of image formats etc so this risk is delegated
- No use of WebKit
- No use of PolicyKit

- No significant cppcheck results
- A couple significant Coverity results which should be relatively easy to
  fix - I have reported these upstream at
  https://github.com/alexpevzner/sane-airscan/issues/131 but the two most
  important ones are:

-----
airscan-http.c:1275
  Type: Uninitialized pointer read (UNINIT)

airscan-http.c:1261:
  1. var_decl: Declaring variable "parser" without initializer.
airscan-http.c:1265:
  2. path: Condition "skip_line", taking true branch.
airscan-http.c:1267:
  3. path: Condition "s", taking true branch.
airscan-http.c:1275:
  4. uninit_use_in_call: Using uninitialized value "parser.data" when calling "http_parser_init".
http_parser.c:2220:
  4.1. read_parm_fld: Reading a parameter field.
------
airscan-os.c:109
  Type: String not null terminated (STRING_NULL)

airscan-os.c:79:
  1. string_null_argument: Function "readlink" does not terminate string "*os_progname_buf". [Note: The source code implementation of the function has been overridden by a builtin model.]
airscan-os.c:80:
  2. path: Condition "rc < 0", taking false branch.
airscan-os.c:109:
  3. string_null: Passing unterminated string "os_progname_buf" to "strrchr", which expects a null-terminated string.
------

- No significant shellcheck results
- No significant bandit results

I noticed licensecheck couldn't find a license on most of the source file -
should this be explicitly listed in each?

Security team ACK for promoting sane-airscan to main conditional on the
unit tests being plumbed into the build or autopkgtests added which at
least run these unit tests instead if they can't easily be run at
compile-time.