Skip to content

Commit

Permalink
Add PHP base-convert-loses-precision (#3307) (#3308)
Browse files Browse the repository at this point in the history
* Add PHP base-convert-loses-precision

The function base_convert uses 64-bit numbers internally, and does not correctly convert large numbers.
It is not suitable for random tokens such as those used for session tokens or CSRF tokens.

* PHP base-convert-loses-precision: Correctly name rule in test

Co-authored-by: Sjoerd Langkemper <[email protected]>
  • Loading branch information
r2c-argo[bot] and Sjord authored Feb 20, 2024
1 parent 0a3063a commit 6667002
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 0 deletions.
80 changes: 80 additions & 0 deletions php/lang/security/base-convert-loses-precision.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<?php

$command = $secret = $counter = $uid = $totalColors = $i = $row = $value = $numeric = $string = '1';
$getRandomNumber = 'rand';

// ok: base-convert-loses-precision
var_dump(base_convert("0775", 8, 10));

// ruleid: base-convert-loses-precision
$token = base_convert(bin2hex(hash('sha256', uniqid(mt_rand(), true), true)), 16, 36);

// ruleid: base-convert-loses-precision
base_convert(hash_hmac('sha256', $command . ':' . $token, $secret), 16, 36);

// ruleid: base-convert-loses-precision
$randString = base_convert(sha1(uniqid(mt_rand(), true)), 16, 36);

// ok: base-convert-loses-precision
$token = 'gleez_profiler/'.base_convert($counter++, 10, 32);

// ok: base-convert-loses-precision
$color1Index = base_convert(substr($uid, 0, 2), 16, 10) % $totalColors;

// ruleid: base-convert-loses-precision
$uniqueId = substr(base_convert(md5(uniqid(rand(), true)), 16, 36), 1, 20);

// ruleid: base-convert-loses-precision
$token = base_convert(sha1($i),7, 36);

// ok: base-convert-loses-precision
$id_converted = base_convert($row, 10, 36);

// ok: base-convert-loses-precision
$value = base_convert(substr($value, 2), 16, 10);

// ok: base-convert-loses-precision
base_convert(rand(1, 1000000000), 10, 36);

// ruleid: base-convert-loses-precision
$salt = base_convert(bin2hex(random_bytes(20)), 16, 36);

// taking only 7 hex chars makes it fit into a 64-bit integer
$stringHash = substr(md5($string), 0, 7);
// ok: base-convert-loses-precision
base_convert($stringHash, 16, 10);

$stringHash = substr(md5($string), 0, 8);
// ruleid: base-convert-loses-precision
base_convert($stringHash, 16, 10);

// ruleid: base-convert-loses-precision
$seed = base_convert(md5(microtime().$_SERVER['DOCUMENT_ROOT']), 16, $numeric ? 10 : 35);

$bytes = random_bytes(32);
// ruleid: base-convert-loses-precision
base_convert(bin2hex($bytes), 16, 36);

// ruleid: base-convert-loses-precision
$salt = base_convert(bin2hex(random_bytes(20)), 16, 36);

// ruleid: base-convert-loses-precision
base_convert(bin2hex(hash('sha256', uniqid(mt_rand(), true), true)), 16, 36);

// ruleid: base-convert-loses-precision
base_convert(bin2hex(openssl_random_pseudo_bytes(8)), 16, 36);

// hard to say whether this is OK or not
base_convert(bin2hex($getRandomNumber()), 16, 36);

// ok: base-convert-loses-precision
base_convert(bin2hex(iconv('UTF-8', 'UCS-4', $m)), 16, 10);

// ruleid: base-convert-loses-precision
$salt = base_convert(bin2hex($this->security->get_random_bytes(20)), 16,36);

// ok: base-convert-loses-precision
$currentByteBits = str_pad(base_convert(bin2hex(fread($fp,1)), 16, 2),8,'0',STR_PAD_LEFT);

// ok: base-convert-loses-precision
base_convert(bin2hex(random_bytes(7)), 16, 36);
50 changes: 50 additions & 0 deletions php/lang/security/base-convert-loses-precision.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
rules:
- id: base-convert-loses-precision
message: >-
The function base_convert uses 64-bit numbers internally, and does not correctly convert large numbers.
It is not suitable for random tokens such as those used for session tokens or CSRF tokens.
metadata:
references:
- https://www.php.net/base_convert
- https://www.sjoerdlangkemper.nl/2017/03/15/dont-use-base-convert-on-random-tokens/
category: security
technology:
- php
cwe:
- 'CWE-190: Integer Overflow or Wraparound'
subcategory:
- audit
likelihood: LOW
impact: LOW
confidence: HIGH
languages: [php]
severity: WARNING
mode: taint
pattern-sources:
- pattern: hash(...)
- pattern: hash_hmac(...)
- pattern: sha1(...)
- pattern: md5(...)
- patterns:
- pattern: random_bytes($N)
- metavariable-comparison:
metavariable: $N
comparison: $N > 7
- patterns:
- pattern: openssl_random_pseudo_bytes($N)
- metavariable-comparison:
metavariable: $N
comparison: $N > 7
- patterns:
- pattern: $OBJ->get_random_bytes($N)
- metavariable-comparison:
metavariable: $N
comparison: $N > 7
pattern-sinks:
- pattern: base_convert(...)
pattern-sanitizers:
- patterns:
- pattern: substr(..., $LENGTH)
- metavariable-comparison:
metavariable: $LENGTH
comparison: $LENGTH <= 7

0 comments on commit 6667002

Please sign in to comment.