Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Next #19

Closed
wants to merge 14 commits into from
Closed

Next #19

wants to merge 14 commits into from

Conversation

serhepopovych
Copy link
Contributor

Hi. With this patch series I would like to introduce/extend following functionality of the cve-check-tool:

Most of the effort applied to improve database and check results consistency in multi user/instance environments. One of good example of such environment is Image Security Framework (ISAFW) used with meta-security-isafw custom layer in YOCTO project.

All patches tested before submission and seems to compile & work correctly in my test cases addressing all issues I found while working for ISAFW improvements.

  • Support for specifying database location in filesystem via command line option (commits f432f9d and 652ae0a)
  • Changes to the output plugin logic: now all plugins (currently HTML, CSV and CLI) use stdout to generate reports. This eases report processing. Error messages now sent to the stderr to not interfere with report data. (commits 9cfb9be, a7a5b59, eefa193, cea2b64)
  • Small code clean up in prepare for the upcoming changes (commit cea2b64)
  • Database locking extended to serialize all access to the database. Before this patch only parallel updates protected from each other, while readers still could access data in the middle of update process. Patch introduces full shared vs exclusive POSIX advisory locking, where exclusive locking
    used only for database update process, and shared locking used by multiple instances for database checks (commit 568ac0f).
  • Introduce marker file in the database directory to correctly handling database partial updates (commits 567126f, 0f4bb46).
  • Fixes to ensure database directory exists before working with locks (commit fcbef7f).
  • Small series of changes to consolidate out-of-memory (oom) error handling in update_db(). While
    it uses goto statement it is pretty clean to read and I think makes code readable a bit. Statement is used in the similar way as with Linux kernel error path handling, so I think it is ok here (commit 705a2be).
  • Check NVD XML files integrity before processing. Among with XML DATA files NVD provides META files. These files are colon separated list of key:value. One of the key provides sha256 digest of the uncompressed XML data file, so we can compute sha256 digest on fetched/unpacked data and compare it to the reference value from the "sha256" key in META file.
    These checksumming does not ensure source authenticity (e.g. like digital signatures), but it is
    used to catch partial downloads. Also fetch/unpack/update mechanism was changed to be more robust and retry at least once (commits 1b1bee3, 5c6cf61, b6ccdd8).

Sergey Popovich added 14 commits January 5, 2016 13:19
Both fetched NVD XMLs and SQLite3 database is
is quite large enough (> 500MB).

This isn't concern in two cases:
  * You have lot of storage space at the home directory.
  * Home directory isn't managed with some kind of backup tool.

First is not always true as in some configurations users
home directory might reside on the shared filesystem (e.g.
NFS) with quotas applied. Thus making impossible to use
home directory for database, while enough space provided at
the different location could be used.

While using filesystem symlinks or bind mounts solves
problem, this seems not feasible for every configurations.

Second could increase home directory backup size dramatically.

Now since we have database update serialized we can place
database in the shared location, so few instances of
the tool could access it simultaneously, with only one
instance at the time able to update database.

This saves lots of space in some configurations and makes
possible to place database in filesystem pathes where
redundancy isn't concern (e.g. /tmp, no backups of the area).

Signed-off-by: Sergey Popovich <[email protected]>
Allow user to supply path to the NVD directory in
filesystem as optional argument to the cve-check-{tool,update}.

If argument isn't specified defaults is to ~/NVDS.

This enables sharing of NVD for better storage utilization
and improved results consistency.

Signed-off-by: Sergey Popovich <[email protected]>
Before this patch we have following output logic
managed by the --csv and --no-html options:

  1. CSV and HTML reports are mutually exclusive
  2. CSV uses stdout as reports destinations
  3. CLI is only used when CSV isn't enabled
  4. The only option to disable CLI is to use CSV mode.

Change --csv and --no-html options handling so
that only single output plugin could be used at
the time.

This patch is prepare to make use stdout in all
report plugins as output destination.

Signed-off-by: Sergey Popovich <[email protected]>
CSV already uses stdout as output destination,
make CLI to use it too.

This splits error reporting and actual
data reports in CLI output plugin making it
easily possible to redirect errors to other
destinations.

Signed-off-by: Sergey Popovich <[email protected]>
Now CSV and CLI plugins use stdout for their report output,
HTML writes output in single report.html file. File created
in filesystem at the current working directory path which
might be confusing to the other tools looking for report.html
since cwd path might change from one build to another
(e.g. in automated build environments).

Adjust HTML report plugin to use stdout to bring it inline
with CSV and CLI plugins to make it possible.

Main user who benefits from this change is Image Security
Analyser Framework (ISAFW) and their corresponding YOCTO
meta layer, since now they able to store HTML report in
any place (e.g. their timestamped logging directory).

Signed-off-by: Sergey Popovich <[email protected]>
  * Make NVD database file and dir name available to other modules
  * Remove unused YEAR_START and URI_PREFIX from the core.c
  * Move DEF_AUTOFREE(char, free) to the util.h to make it reusable

Signed-off-by: Sergey Popovich <[email protected]>
One of the obvious reason for this change is to ensure check is
always performed against the same set of the CVE data in SQLite3
database, excluding the case where one instance updates database
and another uses it for CVE checks.

This behaviour is observed with Image Security Analyser Framework
(ISAFW) running with bitbake on YOCTO project builds in case where
no database exists or it's content is outdated, first instance
starts database update while other instances use database at the
same time, which they consided as up-to-date (i.e. with
update_required() returning false), for package checking. This
leads for incorrect results.

The approach to address descibled issues and other possible is to
extend advisory file locking usage to shared locks for read
database access and exclusive locking for write access, so
database updates only possible when there is no readers and only
one writer.

Introduce and use common API to work with advisory locking in both
cve-check-tool and cve-check-update. Now if there are readers and
database is out-of-date next instance can not aquire write lock
for update it waits to aquire it. If there are single writer all
reader instances can not aquire read lock they will wait for
writer to complete the update.

Lock timeout is kept from original database update locking
implementation, while adding command line argument option to
overwrite it at runtime might be useful.

Signed-off-by: Sergey Popovich <[email protected]>
This code could be reused later to allocate other dot file names.

Signed-off-by: Sergey Popovich <[email protected]>
While sqlite is resistent to the database update failres due to
update process interruption there is other curcumstances affecting
checking package against CVE database.

For example if database update in progress, CVEs is loaded for
2004 year and load for 2005 year started, process performing
update crashes or user interrupted.

On next invocation database might be seen as up-to-date (due to
database access time), sqlite engine performs recovery of the
database to the last usable state, so database isn't corrupted,
but package checking against partial data loaded resulting for
inaccurate results (e.g. package version is never than CVEs in
database, as result everything seems is ok, for 2004 year...).

To address this issue create update timestamp file in the database
directory on update begining and remove such file only when database
is fully updated. If such file present in the directory database
requires update before use.

Remove database file if timestamp file present or database file
is zero sized to force data load.

Also make sure we always update access and modify time of the
database file to the current time.

Signed-off-by: Sergey Popovich <[email protected]>
Overwise we might fail to create database lock file.

Signed-off-by: Sergey Popovich <[email protected]>
There are couple of places in update_db() where we can get
memory allocation failures. Consolidate such error handling
in single place and make code more readable.

Check for oom from call to g_strdup_printf() in update_db().

Allocate database directory and update file paths at the
beginning of the update procedure, so oom conditions
catched before we attempt to establish lock or create
update dot file.

Move code checking for valid file descriptor from update_end()
up to the caller.

Signed-off-by: Sergey Popovich <[email protected]>
…ions

Introduce nvdcve_get_fname() to get name of the NVD file and
nvdcve_get_pname() build absolute path to the NVD file using
g_build_path().

This code can be reused in the future to get other NVD
files (e.g. META).

Also check for result from introduced functions and
g_strdup_printf(). Consolidate similar error pathes into one.

Signed-off-by: Sergey Popovich <[email protected]>
To reduce one level of indendation and
make update_db() code more readable.

Signed-off-by: Sergey Popovich <[email protected]>
National Vulnerability Database provides vulnerability information
in the form of XML files. These files are available in compressed
GZ or ZIP forms via HTTP/HTTPS protocol.

Fetching of the NVD XML could be incomplete for various reasons
(e.g. network reachability issues, interrupted download, etc).

To address this NVD provides META files for each XML data file.
This metadata provides last modification date/time, size of the
data in compressed/uncompressed forms and sha256 digest of the
uncompressed data. File is regular, plain text file with two
fields separated by the colon. First field must not contain
colon (as it is used as field separator), but second may contain it.

Introduce routines to parse NVD META files to get sha256 digest.
Compute digest on the fetched/unpacked data and compare it with
one from NVD META file.

If NVD XML files fetched we validate their integrity by calculating
sha256 on the uncompressed NVD XML file and comparing the result
with value from NVD META sha256 field.

If decompression fails or digest does not match we try to fetch NVD
data again and check the result, if that fails again we raise error.

If fetching of NVD file(s) was skipped (e.g. because no modification
is done to the file(s) on site since their local mtime value) and
data integrity isn't ok we try to uncompress and check digest again,
if that fails we try to refetch and check again.

While it might be reasonable to keep at least compressed XML
file and modify fetcher to resume failed downloads, this is
not preffered as solution due to the bugs in cURL related
to the HTTP 416 Requested Range Not Satisfiable error handling,
so make any failed downloads to start from the beginning.

Signed-off-by: Sergey Popovich <[email protected]>
@ikeydoherty
Copy link
Contributor

Sorry, going to have to merge Fixes into 5.5, its a bad bug we're seeing

@serhepopovych
Copy link
Contributor Author

Now I perform heavy rebase to the upstream master and prefer to close this pool request in favour to the new one found in next2.

@serhepopovych serhepopovych deleted the next branch January 20, 2016 09:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants