Project

General

Profile

Actions

Bug #5539

open

landlock: coverity warnings

Added by Victor Julien almost 2 years ago. Updated 5 months ago.

Status:
In Review
Priority:
High
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

** CID 1514671:  Error handling issues  (CHECKED_RETURN)
/src/util-landlock.c: 181 in LandlockSandboxing()

________________________________________________________________________________________________________
*** CID 1514671:  Error handling issues  (CHECKED_RETURN)
/src/util-landlock.c: 181 in LandlockSandboxing()
175     }
176     
177     void LandlockSandboxing(SCInstance *suri)
178     {
179         /* Read configuration variable and exit if no enforcement */
180         int conf_status;
>>>     CID 1514671:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "ConfGetBool" without checking return value (as is done elsewhere 30 out of 31 times).
181         ConfGetBool("security.landlock.enabled", &conf_status);
182         if (!conf_status) {
183             SCLogConfig("Landlock is not enabled in configuration");
184             return;
185         }
186         struct landlock_ruleset *ruleset = LandlockCreateRuleset();

** CID 1514670:  Security best practices violations  (TOCTOU)
/src/util-landlock.c: 204 in LandlockSandboxing()

________________________________________________________________________________________________________
*** CID 1514670:  Security best practices violations  (TOCTOU)
/src/util-landlock.c: 204 in LandlockSandboxing()
198         if (suri->run_mode == RUNMODE_PCAP_FILE) {
199             const char *pcap_file;
200             ConfGet("pcap-file.file", &pcap_file);
201             char *file_name = SCStrdup(pcap_file);
202             if (file_name != NULL) {
203                 struct stat statbuf;
>>>     CID 1514670:  Security best practices violations  (TOCTOU)
>>>     Calling function "stat" to perform check on "file_name".
204                 if (stat(file_name, &statbuf) != -1) {
205                     if (S_ISDIR(statbuf.st_mode)) {
206                         LandlockSandboxingReadPath(ruleset, file_name);
207                     } else {
208                         LandlockSandboxingReadPath(ruleset, dirname(file_name));
209                     }

** CID 1514669:    (CHECKED_RETURN)
/src/util-landlock.c: 248 in LandlockSandboxing()
/src/util-landlock.c: 200 in LandlockSandboxing()

________________________________________________________________________________________________________
*** CID 1514669:    (CHECKED_RETURN)
/src/util-landlock.c: 248 in LandlockSandboxing()
242             } else {
243                 LandlockSandboxingWritePath(ruleset, LOCAL_STATE_DIR "/run/suricata/");
244             }
245         }
246         if (suri->sig_file_exclusive == FALSE) {
247             const char *rule_path;
>>>     CID 1514669:    (CHECKED_RETURN)
>>>     Calling "ConfGet" without checking return value (as is done elsewhere 87 out of 89 times).
248             ConfGet("default-rule-path", &rule_path);
249             if (rule_path) {
250                 LandlockSandboxingReadPath(ruleset, rule_path);
251             }
252         }
253     
/src/util-landlock.c: 200 in LandlockSandboxing()
194         if (stat(ConfigGetDataDirectory(), &sb) == 0) {
195             LandlockSandboxingAddRule(ruleset, ConfigGetDataDirectory(),
196                     _LANDLOCK_SURI_ACCESS_FS_WRITE | _LANDLOCK_ACCESS_FS_READ);
197         }
198         if (suri->run_mode == RUNMODE_PCAP_FILE) {
199             const char *pcap_file;
>>>     CID 1514669:    (CHECKED_RETURN)
>>>     Calling "ConfGet" without checking return value (as is done elsewhere 87 out of 89 times).
200             ConfGet("pcap-file.file", &pcap_file);
201             char *file_name = SCStrdup(pcap_file);
202             if (file_name != NULL) {
203                 struct stat statbuf;
204                 if (stat(file_name, &statbuf) != -1) {
205                     if (S_ISDIR(statbuf.st_mode)) {

The retval checking is pretty trivial. Not sure how the TOCTOU would be solved in this case. @Philippe Antoine any thoughts?

Subtasks 1 (1 open0 closed)

Bug #6541: landlock: coverity warnings (7.0.x backport)AssignedEric LeblondActions
Actions #1

Updated by Philippe Antoine almost 2 years ago

Would realpath work here ?

Actions #2

Updated by Philippe Antoine almost 2 years ago

No satisfying answer.
open the file and use fstat rather than stat may silence coverity...

Actions #3

Updated by Victor Julien over 1 year ago

  • Target version changed from 7.0.0-beta1 to 7.0.0-rc1
Actions #4

Updated by Victor Julien over 1 year ago

  • Target version changed from 7.0.0-rc1 to 7.0.0-rc2
Actions #5

Updated by Philippe Antoine over 1 year ago

ping @Eric Leblond What are the news on this ?

Actions #6

Updated by Eric Leblond over 1 year ago

Giving it some time now.

Actions #7

Updated by Eric Leblond over 1 year ago

I've pushed https://github.com/regit/suricata/tree/landlock-coverity using the fstat solution but I don't know if coverity will go for a test. The fix is not really a fix.

Actions #8

Updated by Philippe Antoine over 1 year ago

  • Status changed from Assigned to In Review
Actions #9

Updated by Philippe Antoine over 1 year ago

I think the TOCTOU is indeed not really a problem as LandlockSandboxingReadPath does good checks later

Actions #10

Updated by Victor Julien about 1 year ago

  • Target version changed from 7.0.0-rc2 to 7.0.0
Actions #11

Updated by Victor Julien about 1 year ago

  • Priority changed from Normal to High
Actions #12

Updated by Victor Julien about 1 year ago

  • Target version changed from 7.0.0 to 7.0.1
Actions #13

Updated by Victor Julien 10 months ago

  • Target version changed from 7.0.1 to 7.0.2
Actions #14

Updated by Victor Julien 9 months ago

Any update @Eric Leblond?

Actions #15

Updated by Eric Leblond 9 months ago

I will get a look at this this week.

Actions #16

Updated by Victor Julien 9 months ago

  • Target version changed from 7.0.2 to 7.0.3
Actions #17

Updated by Victor Julien 8 months ago

  • Target version changed from 7.0.3 to 8.0.0-beta1
  • Label Needs backport to 7.0 added
Actions #18

Updated by OISF Ticketbot 8 months ago

  • Subtask #6541 added
Actions #19

Updated by OISF Ticketbot 8 months ago

  • Label deleted (Needs backport to 7.0)
Actions

Also available in: Atom PDF