pySCG bugfix for CWE-191 as per #835 #838
Conversation
Signed-off-by: Helge Wehder <[email protected]>
|
I am reviewing this. |
| from datetime import datetime, timedelta | ||
|
|
||
|
|
||
| def get_datetime(currtime: datetime, hours: int): |
There was a problem hiding this comment.
I am getting this error with Pylint, might be a good idea to look into this:
Redefining name 'currtime' from outer scope (line 26)PylintW0621:redefined-outer-name.
This also applies for hours:
Redefining name 'hours' from outer scope (line 43)PylintW0621:redefined-outer-name
| try: | ||
| result = get_datetime(currtime, hours) | ||
| print(f"{hours} OK, datetime='{result}'") | ||
| except Exception as exception: |
There was a problem hiding this comment.
I am getting this error in Pylint, saying that the exception is too general:
Catching too general exception ExceptionPylintW0718:broad-exception-caught
| ## Compliant Solution | ||
|
|
||
| This `compliant02.py` solution handles `OverflowError` Exception when a too large value is given to `get_time_in_future`. | ||
| This `compliant02.py` solution is peventing `OverflowError` exception in `libpython` by safeguarding the upper and lowern limits in the provided `hours`. Upper and lower limit for `currtime` as well as inputsanitation and secure logging are missing and must be added when interfacing with a lesser trusted entity. |
There was a problem hiding this comment.
| This `compliant02.py` solution is peventing `OverflowError` exception in `libpython` by safeguarding the upper and lowern limits in the provided `hours`. Upper and lower limit for `currtime` as well as inputsanitation and secure logging are missing and must be added when interfacing with a lesser trusted entity. | |
| This `compliant02.py` solution is preventing `OverflowError` exception in `libpython` by safeguarding the upper and lower limits in the provided `hours`. Upper and lower limit for `currtime` as well as input sanitization and secure logging are missing and must be added when interfacing with a lesser trusted entity. |
| except OverflowError as _: | ||
| return "Number too large to set time in future " + str(hours_in_future) | ||
|
|
||
| def get_datetime(currtime: datetime, hours: int): |
There was a problem hiding this comment.
Again seeing the outer scope warning:
Redefining name 'currtime' from outer scope (line 49)PylintW0621:redefined-outer-name
Redefining name 'hours' from outer scope (line 66)PylintW0621:redefined-outer-name
| try: | ||
| result = get_datetime(currtime, hours) | ||
| print(f"{hours} OK, datetime='{result}'") | ||
| except Exception as exception: |
There was a problem hiding this comment.
Again with the general exception warning, not sure if it is relevant a I believe you want to catch all exceptions:
Catching too general exception ExceptionPylintW0718:broad-exception-caught
| ``` | ||
|
|
||
| The `compliant02.py` example is protecting the lower level c-lib from an `OverflowError` by setting boundaries for valid values in `hours`. Similar issues occure with any functionality provided through the operating system. |
There was a problem hiding this comment.
| The `compliant02.py` example is protecting the lower level c-lib from an `OverflowError` by setting boundaries for valid values in `hours`. Similar issues occure with any functionality provided through the operating system. | |
| The `compliant02.py` example is protecting the lower level c-lib from an `OverflowError` by setting boundaries for valid values in `hours`. Similar issues occur with any functionality provided through the operating system. |
|
|
||
|
|
||
| ##################### | ||
| # attempting to exploit above code example | ||
| ##################### | ||
| print(get_time_in_future(23**74)) | ||
| datetime.fromtimestamp(0) | ||
| currtime = datetime.fromtimestamp(1) # 1st Jan 1970 |
There was a problem hiding this comment.
Maybe we should change the name of the "currtime" variable to be different than the input parameter into the "get_datetime()" function. It's better practice
| currtime = datetime.fromtimestamp(1) # 1st Jan 1970 | |
| current_time = datetime.fromtimestamp(1) # 1st Jan 1970 |
| for hours in hours_list: | ||
| try: | ||
| result = get_datetime(currtime, hours) | ||
| print(f"{hours} OK, datetime='{result}'") | ||
| except Exception as exception: | ||
| print(f"{hours} {repr(exception)}") |
There was a problem hiding this comment.
Changed the name to "hours_shift" as its the amount of hours we are shifting in time? Just to try and keep the variable name different from the input function... maybe you can think of a better name though... or change the name in the input variable in "get_datetime()"
| for hours in hours_list: | |
| try: | |
| result = get_datetime(currtime, hours) | |
| print(f"{hours} OK, datetime='{result}'") | |
| except Exception as exception: | |
| print(f"{hours} {repr(exception)}") | |
| for hours_shift in hours_list: | |
| try: | |
| result = get_datetime(currtime, hours_shift) | |
| print(f"{hours_shift} OK, datetime='{result}'") | |
| except Exception as exception: | |
| print(f"{hours_shift} {repr(exception)}") |
Trying to address issues reported in #835 , the excessive attack section might be to much but covers all boundaries I discovered.