Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues in the hidsim-plugin, which were identified by static code analysis #1255

Merged
merged 1 commit into from
Jul 10, 2021

Conversation

jgru
Copy link
Contributor

@jgru jgru commented Jul 10, 2021

Dear Tamas,

this PR addresses the following issues, which were identified by Coverity, and tries to solve those:

  • CID 350859: Fix usage of strdup(...) with unterminated string
  • CID 350856: Fix resource leaks in get_display_dimensions(...)
  • CID 350857: Fix resource leaks in get_display_dimensions(...)
  • CID 350854: Fix unterminated strings in qmp_init_conn()
  • CID 350852: Fix potential buffer overflow in get_display_dimensions(...)
  • CID 350851: Fix submission of tainted data to usleep(...)
  • CID 350855: Fix calculations with tainted scalar get_display_dimensions(...)
  • CID 350860: Replace rand() with std::mt19937 to fix WEAK_CRYPTO-warning
  • CID 350853: Replace rand() with std::mt19937 to fix WEAK_CRYPTO-warning
  • CID 350858: Replace rand() with std::mt19937 to fix WEAK_CRYPTO-warning

Besides that, a few minor clean-ups have been performed.
Thank you already in advance for considering this PR.
I hope, that this patch will solve the problems introduced by #1245.

Best regards
Jan

@drakvuf-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@tklengyel
Copy link
Owner

@drakvuf-jenkins This is OK to test

@jgru jgru force-pushed the fix-coverity-issues-of-hidsim branch from d80359b to 8ddf6e7 Compare July 10, 2021 13:39
* Fix usage of strdup(...) with unterminated string (CID 350859)

Fix usage of strdup(...) with an unterminated string in  l.25
qmp_connection.cpp, which was recorded as CID 350859.

* Fix resource leaks in get_display_dimensions(...) (CID 350857)

Fix resource leaks in get_display_dimensions(...) of the file
/src/plugins/hidsim/hid_injection.cpp, which was identified as
CID 350857 by coverity.

* Fix unterminated strings in qmp_init_conn() (CID 350854)

Fix issues with unterminated strings in qmp_init_conn() of the file
src/plugins/hidsim/qmp_connection.cpp, which were identified
as CID 350854 by coverity.

* Fix potential buffer overflow (CID 350852)

Fix a potential buffer overflow vulnerability in l. 208 of
get_display_dimensions in file src/plugins/hidsim/hid_injection.cpp,
which was identified as CID 350852 by coverity

* Fix submission of tainted data to usleep (CID 350851)

Fix the submission of tainted data to usleep(...) in l. 615
and 677 of run_template_injection() in src/plugins/hidsim/\
hid_injection.cpp, which was identified as CID 350851 by
coverity.

* Fix calculations with tainted scalar (CID 350855)

Fix calculations with tainted scalar in l. 217 f. in
get_display_dimensions(...) of src/plugins/hidsim/hid_injection.cpp,
which was identified as CID 350855 by coverity.

* Replace rand with mt19937 to fix WEAK_CRYPTO-issues (CID 350860)

Replace rand() with Mersenne Twister implementation (std::mt19937)
to fix DC.WEAK_CRYPTO-issues, which were identified as CID 350860,
350858 and 350853 by coverity.

* Clean-up coding style
@jgru jgru force-pushed the fix-coverity-issues-of-hidsim branch from 8ddf6e7 to 934e565 Compare July 10, 2021 15:21
@tklengyel tklengyel merged commit 15d788c into tklengyel:master Jul 10, 2021
@tklengyel
Copy link
Owner

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)


** CID 350879:    (USE_AFTER_FREE)
/src/plugins/hidsim/hid_injection.cpp: 228 in get_display_dimensions(qmp_connection *, dimensions *)()
/src/plugins/hidsim/hid_injection.cpp: 242 in get_display_dimensions(qmp_connection *, dimensions *)()


________________________________________________________________________________________________________
*** CID 350879:    (USE_AFTER_FREE)
/src/plugins/hidsim/hid_injection.cpp: 228 in get_display_dimensions(qmp_connection *, dimensions *)()
222             fclose(fr);
223             ret = -1;
224         }
225     
226         /* Retrieves screen dimensions by reading screen dump  */
227         uint32_t width = 0, height = 0;
>>>     CID 350879:    (USE_AFTER_FREE)
>>>     Calling "fscanf" uses file handle "fr" after closing it.
228         matches = fscanf(fr, "%" SCNd32 "%" SCNd32, &width, &height);
229     
230         if (matches != 2 )
231         {
232             fprintf(stderr, "[HIDSIM]: Error reading .ppm-files metadata\n");
233             fclose(fr);
/src/plugins/hidsim/hid_injection.cpp: 242 in get_display_dimensions(qmp_connection *, dimensions *)()
236     
237         /* Sanitizes read values; no need for a screen size larger than 65k */
238         dims->width = (uint16_t)(width > UINT16_MAX ? UINT16_MAX : width);
239         dims->height = (uint16_t)(height > UINT16_MAX ? UINT16_MAX : height);
240     
241         /* Clean up */
>>>     CID 350879:    (USE_AFTER_FREE)
>>>     Calling "fclose" uses file handle "fr" after closing it.
242         fclose(fr);
243     
244         /* Removes temp file generated by the screendump-command */
245         if (remove(tmp_path) != 0)
246             fprintf(stderr, "[HIDSIM]: Error deleting screen dump %s\n", tmp_path);
247     

jgru added a commit to jgru/drakvuf that referenced this pull request Jul 11, 2021
* Fix use after free of filestream (CID 350856)

Fix use after free in l. 731 of hid_cleanup(), which was identified
as CID 350856 by coverity.

* Fix use after close in get_display_dimensions(...) (CID 350879)

Fix use after close of file stream in l. 242 in get_display_\
dimensions(...), which was identified as CID 350879 by coverity.

* Fix untrusted divisor in get_display_dimensions (CID 350855)

Fix untrusted divisor in l. 249 of get_display_dimensions(...), which
was identified CID 350855 by coverity. Furthermore, guard against
unnecessary big screensizes.
@jgru
Copy link
Contributor Author

jgru commented Jul 11, 2021

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)
[...]  

Addressed in PR #1256

tklengyel pushed a commit that referenced this pull request Jul 11, 2021
* Fix use after free of filestream (CID 350856)

Fix use after free in l. 731 of hid_cleanup(), which was identified
as CID 350856 by coverity.

* Fix use after close in get_display_dimensions(...) (CID 350879)

Fix use after close of file stream in l. 242 in get_display_\
dimensions(...), which was identified as CID 350879 by coverity.

* Fix untrusted divisor in get_display_dimensions (CID 350855)

Fix untrusted divisor in l. 249 of get_display_dimensions(...), which
was identified CID 350855 by coverity. Furthermore, guard against
unnecessary big screensizes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants