flawfinder warnings in mork

RESOLVED INVALID

Status

MailNews Core
Database
RESOLVED INVALID
16 years ago
10 years ago

People

(Reporter: Stephen P. Morse, Assigned: Bienvenu)

Tracking

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

16 years ago
Heikki ran flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 
branch.

flawfinder found 4 warnings in db/mork code (1686-1689). Go through
that list and for each warning:

* If it is false positive, comment here why it is not an issue
* If it is a real issue, make patch for it here and let's get them checked in

In addition to checking the branch, also check the trunk.

1686) db/mork/src/morkConfig.cpp:69 [4] (race) access: this usually indicates a 
security flaw. If an attacker can change anything along the path between the 
call to access() and the file's actual use (e.g., by moving files), the attacker 
can exploit the race condition. Set up the correct permissions (e.g., using 
setuid()) and try to open the file directly.

1687) db/mork/src/morkConfig.cpp:76 [4] (race) access: this usually indicates a 
security flaw. If an attacker can change anything along the path between the 
call to access() and the file's actual use (e.g., by moving files), the attacker 
can exploit the race condition. Set up the correct permissions (e.g., using 
setuid()) and try to open the file directly.

1688) db/mork/src/morkConfig.cpp:80 [4] (race) access: this usually indicates a 
security flaw. If an attacker can change anything along the path between the 
call to access() and the file's actual use (e.g., by moving files), the attacker 
can exploit the race condition. Set up the correct permissions (e.g., using 
setuid()) and try to open the file directly.

1689) db/mork/src/morkConfig.h:169 [4] (buffer) strcpy: does not check for 
buffer overflows. Consider using strncpy or strlcpy.
(Reporter)

Updated

16 years ago
Blocks: 148251
(Assignee)

Comment 1

16 years ago
for 1,2, and 3, there isn't a call to access; it's just a variable called
access. so I'd have to say those are bogus warnings.

for 4, there is only one call using MORK_STRCPY (it's a macro), and that call
allocates the block it's strcpying into:

morkEnv::CopyString(nsIMdbHeap* ioHeap, const char* inString)
{
  char* outString = 0;
  if ( ioHeap && inString )
  {
    mork_size size = MORK_STRLEN(inString) + 1;
    ioHeap->Alloc(this->AsMdbEnv(), size, (void**) &outString);
    if ( outString )
      MORK_STRCPY(outString, inString);
  }
  else
    this->NilPointerError();
  return outString;
}

so, unless I'm misunderstand these, I'd say this bug is invalid.
(Reporter)

Comment 2

16 years ago
4 more flawfinder warnings (4067-4070)

4067) db/mork/src/morkConfig.cpp:69 [4] (race) access: this usually indicates a
security flaw. If an attacker can change anything along the path between the
call to access() and the file's actual use (e.g., by moving files), the attacker
can exploit the race condition. Set up the correct permissions (e.g., using
setuid()) and try to open the file directly.

4068) db/mork/src/morkConfig.cpp:76 [4] (race) access: this usually indicates a
security flaw. If an attacker can change anything along the path between the
call to access() and the file's actual use (e.g., by moving files), the attacker
can exploit the race condition. Set up the correct permissions (e.g., using
setuid()) and try to open the file directly.

4069) db/mork/src/morkConfig.cpp:80 [4] (race) access: this usually indicates a
security flaw. If an attacker can change anything along the path between the
call to access() and the file's actual use (e.g., by moving files), the attacker
can exploit the race condition. Set up the correct permissions (e.g., using
setuid()) and try to open the file directly.

4070) db/mork/src/morkConfig.h:169 [4] (buffer) strcpy: does not check for
buffer overflows. Consider using strncpy or strlcpy. 
Steve, Flawfinder seems to report some of the errors more than once. If you see
several *non-consecutive sections* of warnings for the same module, check if
they are for the same warnings than before.

Timeless, if inString could be non-null-terminated, this function would blow up.
So do at least a trivial check of where the value comes in these situations
(like was it created in this function above). If the chain of where the value
comes from is longer it can fast become impossible to check. In this case there
are several callers where the value also gets passed in, so I am assuming that
it will be null-terminated (would be too difficult to check).

So, I agree with timeless' evaluation and I am marking this invalid.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → INVALID
Heh, sorry, meant bienvenu :)
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.