TingPing / hexchat-otr

Off the record plugin for HexChat
GNU General Public License v2.0
33 stars 9 forks source link

Address issues found by flawfinder? #12

Closed petterreinholdtsen closed 8 years ago

petterreinholdtsen commented 8 years ago

Hi. I ran flawfinder on the source code, and it found a few issues. Are you aware of these issues? Some of them seem to be false positives.

$ flawfinder -Q -c .
Flawfinder version 1.31, (C) 2001-2014 David A. Wheeler.
Number of rules (primarily dangerous function names) in C/C++ ruleset: 169
./src/hexchat_otr.c:7:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
static char set_policy[512] = IO_DEFAULT_POLICY;
./src/hexchat_otr.c:8:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
static char set_policy_known[512] = IO_DEFAULT_POLICY_KNOWN;
./src/hexchat_otr.c:9:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
static char set_ignore[512] = IO_DEFAULT_IGNORE;
./src/hexchat_otr.c:178:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char newmsg[512];
./src/hexchat_otr.c:210:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char nick[256];
./src/hexchat_otr.c:334:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char msg[LOGMAX], *s = msg;
./src/otr.h:157:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char better_msg_two[256]; /* what the second line of the "better"
./src/otr_ops.c:37:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char fullname[1024];
./src/otr_ops.c:135:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char ownfp[45], peerfp[45];
./src/otr_util.c:150:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[256];
./src/otr_util.c:195:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char fp[41];
./src/otr_util.c:237:  [2] (buffer) sprintf:
  Does not check for buffer overflows (CWE-120). Use sprintf_s, snprintf, or
  vsnprintf. Risk is low because the source has a constant maximum length.
                sprintf (fp + i * 2, "%02x",
./src/otr_util.c:259:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[128];
./src/otr_util.c:325:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[128];
./src/otr_util.c:425:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[128];
./src/otr_util.c:486:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[128];
./src/otr_util.c:525:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[128];
./src/otr_util.c:620:  [2] (buffer) char:
  Statically-sized arrays can be improperly restricted, leading to potential
  overflows or other issues (CWE-119:CWE-120). Perform bounds checking, use
  functions that limit length, or ensure that the size is larger than the
  maximum possible length.
    char accname[256];
./src/otr_key.c:99:  [1] (buffer) read:
  Check buffer boundaries if used in a loop including recursive loops
  (CWE-120, CWE-20).
    read (g_io_channel_unix_get_fd (kg_st.ch[0]), &err, sizeof(err));
./src/otr_util.c:586:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
                strlen (secret));
./src/otr_util.c:594:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
                strlen (secret));
./src/otr_util.c:602:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
            strlen (secret));
./src/otr_util.c:655:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
        if ((strlen (msg) > OTR_MAX_MSG_SIZE) && (msg[strlen (msg) - 1] != '.') && (msg[strlen (msg) - 1] != ','))
./src/otr_util.c:655:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
        if ((strlen (msg) > OTR_MAX_MSG_SIZE) && (msg[strlen (msg) - 1] != '.') && (msg[strlen (msg) - 1] != ','))
./src/otr_util.c:655:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
        if ((strlen (msg) > OTR_MAX_MSG_SIZE) && (msg[strlen (msg) - 1] != '.') && (msg[strlen (msg) - 1] != ','))
./src/otr_util.c:659:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
                   strlen (coi->msgqueue));
./src/otr_util.c:668:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
    else if (strstr (msg, "?OTR:") && (strlen (msg) > OTR_MAX_MSG_SIZE) && (msg[strlen (msg) - 1] != '.') && (msg[strlen (msg) - 1] != ','))
./src/otr_util.c:668:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
    else if (strstr (msg, "?OTR:") && (strlen (msg) > OTR_MAX_MSG_SIZE) && (msg[strlen (msg) - 1] != '.') && (msg[strlen (msg) - 1] != ','))
./src/otr_util.c:668:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
    else if (strstr (msg, "?OTR:") && (strlen (msg) > OTR_MAX_MSG_SIZE) && (msg[strlen (msg) - 1] != '.') && (msg[strlen (msg) - 1] != ','))
./src/otr_util.c:672:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
        otr_debug (ircctx, from, TXT_RECEIVE_QUEUED, strlen (msg));
./src/otr_util.c:703:  [1] (buffer) strlen:
  Does not handle strings that are not \0-terminated; if given one it may
  perform an over-read (it could cause a crash if unprotected) (CWE-126).
                   TXT_RECEIVE_IGNORE, strlen (msg), accname, from, msg);

ANALYSIS SUMMARY:

Hits = 31
Lines analyzed = 2337 in approximately 0.04 seconds (59361 lines/second)
Physical Source Lines of Code (SLOC) = 1759
Hits@level = [0]   0 [1]  13 [2]  18 [3]   0 [4]   0 [5]   0
Hits@level+ = [0+]  31 [1+]  31 [2+]  18 [3+]   0 [4+]   0 [5+]   0
Hits/KSLOC@level+ = [0+] 17.6236 [1+] 17.6236 [2+] 10.2331 [3+]   0 [4+]   0 [5+]   0
Dot directories skipped = 2 (--followdotdir overrides)
Minimum risk level = 1
Not every hit is necessarily a security vulnerability.
There may be other security vulnerabilities; review your code!
See 'Secure Programming for Linux and Unix HOWTO'
(http://www.dwheeler.com/secure-programs) for more information.
TingPing commented 8 years ago

Seem like entirely worthless comments and no indication of any real issues...

dramb-lys commented 8 years ago

I've not gone through these issues in detail, but very important to avoid potential security vulnerabilities. Especially when implementing OTR.