Is this a compliant solution for CWE-369?

[andrew-costello]

I am part of a OpenSSF group writing documentation on secure coding in Python and we are scratching our heads on if this is a compliant solution for CWE-369:

# SPDX-FileCopyrightText: OpenSSF project contributors
# SPDX-License-Identifier: MIT
 
""" Compliant Code Example """
 
from decimal import Decimal, ROUND_HALF_UP, Overflow
  
  
def divide(first_number: int, second_number: int):
    '''Function to divide 2 numbers'''
    if second_number == 0:
        raise ZeroDivisionError("TODO: implement zero division error handling here")
    # This operation may cause an overflow due to a large exponent
    try:
        result = Decimal(first_number / second_number)
    except Overflow as e:
        raise Overflow("TODO: implement overflow error handling here") from e
    rounded_result = Decimal(result).quantize(Decimal("0.01"), rounding=ROUND_HALF_UP)
    # Rounding result to 2 decimal place
    return rounded_result
  
  
#####################
# Trying to exploit above code example
#####################
  
for number in (10, 41, 200, -10, 0):
    # Dividing 100 by each number 10, 41, 200, -10, and 0 separately.
    print("-" * 20 + f"divide by {number}" + "-" * 20 + "\n")
    print(f"100 / {number} = ", end="")
  
    print(divide(100, number))

I have decided to go with this approach as Decimal ensures the accuracy of the math, and it also prevents float overflow from occurring.

Would this be a suitable compliant solution?

Overall OpenSSF project on secure coding I work on:

wg-best-practices-os-developers/docs/Secure-Coding-Guide-for-Python at main ·...

[JamesParrott]

If the intent is to return a Decimal (instead of a float or a Fraction), then add a return type annotation for that.

Are you sure CWE-369 is the right identifier? Isn’t there a more specific identifier for overflows?

Anyway, I think this guidance is unnecessary for secure code, and represents a specific design choice that is too opinionated, and overkill in the majority of cases.

I know ensuring denominators are non-zero is common coding good practise. But in environments where crashes and exceptions are acceptable, where’s the security hole in:

>>> 1/0
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    1/0
    ~^~
ZeroDivisionError: division by zero

The user then knows what they did wrong.

Why does the coding guidance need to recommend any more than either:

EAFP

try:
    z = x / y
except ZeroDivisionError:
     raise ZeroDivisionError("TODO: implement zero division error handling here")

or LBYL:

if y == 0:
     raise ZeroDivisionError("TODO: implement zero division error handling here")
z = x / y

or you could use returns.maybe.

Is Python code being run in environments, or compiled to byte code now, on platforms in which dividing by zero results in UB, or in which ZeroDivisionErrors are not handled gracefully ? Even then on such platforms, why is a ZeroDivisionError more of a security hole than a ValueError etc. ?

[barry-scott]

The overflows that are a security risk are typically when an int is used as an index into an array. This in C code for example leads all sorts of ways to attack a program.

But with python that is not possible, by design.

Of course a python extension written in C can have such a bug.
But that is already covered under the C language.

Other sorts of overflow as possible may well affect an application’s correctness.
But will that be a security issue?

Do you have example of bugs what have been fixed in python code where overflow was the root cause?

[oscarbenjamin]

@andrew-costello

we are scratching our heads on if this is a compliant solution for CWE-369:

No one is going to use that code. It does not solve any security problems and also just absurd.

Dividing two integers already raises ZeroDivisionError. You don’t need to check if the divisor is zero and raise ZeroDivisionError because it happens automatically.

The decimal module is not needed either. It adds nothing here. You are dividing the two integers before even converting to decimal which is entirely pointless:

result = Decimal(first_number / second_number)

The code that will be used to divide two integers is x / y if you want a float result or x // y etc if you want to get floor division with integers. With x / y the possible exceptions that can be raised are OverflowError or ZeroDivisionError. With x // y the only possible exception is ZeroDivisionError. Neither of these operations presents any reasonable security risk and no additional code is needed except that in some contexts there may be a need to catch the exceptions but that is very much context-dependent.

The only thing that you could reasonably do for CWE-369 is just to document that it does not apply to any kind of division of integers in Python. If you need to verify that in detail by looking through the CPython source code then go ahead but no alternative Python code suggestion here is going to be useful for anything.

[myteron]

@andrew-costello

CWE-369

The rule is in context with CWE-369, Divide by Zero.

Current conclusion text (BLUF):
When performing division or remainder calculations on integers in Python, a ZeroDivisionError can occur if the divisor is zero. To prevent this, the divisor should be checked to ensure it is not zero before proceeding, or handle the ZeroDivisionError exception if the divisor is zero.

…

## Non-Compliant Code Example
In Python, if you want to divide 2 numbers, the ‘/’ operator is used. The noncompliant01.py code will compute to a ZeroDivisionError when dividing first_number, and second_number, because the divisor (second_number) is 0. Decimal is used as it ensures the accuracy of the math when performing division calculations, and it also prevents float overflow from occurring.

# SPDX-FileCopyrightText: OpenSSF project contributors
# SPDX-License-Identifier: MIT
 
""" Non-Compliant Code Example """
 
from decimal import Decimal, ROUND_HALF_UP
 
def divide(first_number: int, second_number: int):
    '''Function to divide 2 numbers'''
    result = Decimal(first_number / second_number)
    rounded_result = Decimal(result).quantize(Decimal("0.01"), rounding=ROUND_HALF_UP)
    # Rounding result to 2 decimal place
    return rounded_result
 
 
#####################
# Trying to exploit above code example
#####################
 
for number in (10, 41, 200, -10, 0):
    # Dividing 100 by each number 10, 41, 200, -10, and 0 separately.
    print("-" * 20 + f"divide by {number}" + "-" * 20 + "\n")
    print(f"100 / {number} = ", end="")
  
    print(divide(100, number))

[myteron]

@barry-scott

The overflows that are a security risk are typically when an int is used as an index into an array. This in C code for example leads all sorts of ways to attack a program.

Good point, this could lead to a better code example.

But with python that is not possible, by design.

Of course a python extension written in C can have such a bug.
But that is already covered under the C language.

Totally agree, we are currently not doing a great job in capturing this efficiently for the reader, examples are spread across multiple rules. If have a created an issue for that pySCG: Organize rules (CWE base||class||variant) in book sections instead of CWE Pillars
Unfortunately we will have to get everything into GitHub before we can efficiently re-organize the content.

Other sorts of overflow as possible may well affect an application’s correctness.
But will that be a security issue?

Do you have example of bugs what have been fixed in python code where overflow was the root cause?

Only found one ‘questionable’ python related CVE-2017-18207 : The Wave_read._read_fmt_chunk function in Lib/wave.py in Python through 3.6.4 do in relation to CWE-369:

• NIST Base CVSS:3.0 of 6.5 (medium).
• EPSS: 0.16%.

There is a code diff for the patch that includes handling such as

• adding try catch, except struct.error
• if not (sampwidth + 7) // 8
• not self._nchannels

The obvious questions is: “When does an issue qualify for a secure coding rule?”. In most cases we have only a ‘robustness’ issue. DDOS attacks are not as exciting as data leakage and RCEs. Overflows can have potential for both. To be honest, it is very hard to know where to draw the line.

Rg Helge

[JamesParrott]

The usage of decimal.Decimal is leading you guys down an irrelevant rabbit hole now. They’re being suggested in both the compliant solution, and this non-compliant example. The use of it adds nothing but confusion in both cases.

I believe you all mean well, but I’m reacting strongly because other ‘professionals’ who take more of a box ticking approach to security audits, will inevitably point to whatever you come up with as compliant or non-compliant. Currently neither is the case.

I ran it, and all that 'noncompliant" code does is:

--------------------divide by 0--------------------

100 / 0 = Traceback (most recent call last):
  File "C:\noncomp.py", line 25, in <module>
    print(divide(100, number))
          ^^^^^^^^^^^^^^^^^^^
  File "C:\noncomp.py", line 10, in divide
    result = Decimal(first_number / second_number)
                     ~~~~~~~~~~~~~^~~~~~~~~~~~~~~
ZeroDivisionError: division by zero

To which my response is, so what? As a user of a CLI that does this, I now know exactly what to look for to fix.

On my trusty old Casio CFX-9850G I can do:

1 ÷ 0


Ma   ERROR

Dividing by zero in the CPython run time, is no more of an intrinsic security concern, than it is on my 30 year old graphical calculator.

What’s special about a ZeroDivisionError that it needs to be avoided, as opposed to common or garden ValueError, TypeError, ImportError or AttributeErrors that Python users raise all the time in development?

If the code is not running in a “let it crash” Python environment, that can’t handle ZeroDivisionErrors without consequences for security, then firstly that’s the real issue, and secondly that code should sanitise its user inputs, and handle all the other possible types of errors gracefully too (to say the least).

If there was a specific vulnerability in the way some Python runtime handled ZeroDivisionErrors, then that’s an entirely different and an important security concern.

[myteron]

@JamesParrott

ZeroDivisionErrors

Hi @JamesParrott I agree that we shall not act as 'tick-box exercise". If the Python secure coding comes across as such then we will have to change.

At the moment I don’t see any alternative to what we do. Most secure coding documentation is based on lengthy lists of references without actual working code, basically getting away with concepts that do not provide any hard proof. We obviously only post questions here for content that is questionable in the first place, your feedback is very welcome.

We might need to create a parking area in our guide for questionable rules.

[JamesParrott]

Maybe there could just be numerous examples of compliant solutions:

• running the code in a safe environment that raises a divide by zero exception, and handles exceptions gracefully.
• user input checking.
• a Maybe Monad.
• return float('nan') (or requiring zeros to be signed and returning + or - float('inf') accordingly)
• intrinsically non-zero denominators, e.g. len(...) + 1.
• converting the numerator and denominator to a class T on which T(1)/T(0) is valid, (e.g. the Projectively Extended Real Line).

I maintain using the normal CPython run time counts as the first.

But there are other run-times, and even Python compilers now. Perhaps users of micro-Python or circuit Python on embedded devices should be more careful (for safety critical applications, I personally question the choice to use Python in the first place for that).

[andrew-costello]

Thank you everyone for the help.

I am parking up this documentation for now, will visit it later.

[elis.byberi]

I’m not quite seeing the issue with exceptions. They’re not a security risk. In fact, they help make code more reliable and secure.

If you believe there’s a security issue in CPython, please follow the vulnerability disclosure process.