zopefoundation / RestrictedPython

A restricted execution environment for Python to run untrusted code.
http://restrictedpython.readthedocs.io/
Other
456 stars 38 forks source link

separating security messages #278

Open jsmith173 opened 5 months ago

jsmith173 commented 5 months ago

Is it possible to seperate security messages? It seems RestrictedPython uses existing exception types to report a security error.

I have found in compile.py

if result.errors: raise SyntaxError(result.errors)`

When I replace SyntaxError to my new exception type it will solve the problem? Or how could I do that?

icemac commented 5 months ago

We could create a custom subclass of the existing exception classes, because a SyntaxError should stay one to prevent problems with existing code expecting a Python SyntaxError.

icemac commented 5 months ago

A PR is welcome.

d-maurer commented 5 months ago

Michael Howitz wrote at 2024-3-27 23:53 -0700:

We could create a custom subclass of the existing exception classes, because a SyntaxError should stay one to prevent problems with existing code expecting a Python SyntaxError.

I do not see a need for a change -- especially not for SyntaxError: An application could know whether it has used compile or compile_restricted and therefore whether it sees an unrestricted or a restricted SyntaxError.

jsmith173 commented 5 months ago

When I handle 'SyntaxError' in the app how could I check, it is a real syntax error or it is a security exception? Also sometimes security error messages come from 'exec'

d-maurer commented 5 months ago

jsmith173 wrote at 2024-3-28 01:08 -0700:

When I handle 'SyntaxError' in the app how could I check, it is a real syntax error or it is a security exception? Also sometimes security error messages come from 'exec'

I have searched the RestrictedPython sources: there is a single place where RestrictedPython itself raises a SyntaxError: at the end of RestrictedPython.compile.compile_restricted.

You have several options to distinguish this use of SyntaxError from the "normal" ones:

Regarding exceptions from exec

In a previous comment, I have sketched how RestrictedPython works: it transforms the Python source code to implement standard Python features (especially attribute access and subscription) to function calls. This gives an application control over those features by implementing those functions as it needs them.

Thus, if you do not like the exceptions raised by some implementation functions provided by RestrictedPython, you replace those functions by your own implementation which raises the exceptions you like.

You can search the RestrictedPython sources for Error (as word part) to learn which exceptions RestrictedPython uses and which parts you may want to replace by your own implementation functions.

jsmith173 commented 5 months ago

compile_restricted collects the real syntax errors so even if I have a new exception I will have real syntax errors in that.

d-maurer commented 5 months ago

jsmith173 wrote at 2024-3-28 03:50 -0700:

compile_restricted collects the real syntax errors so even if I have a new exception I will have real syntax errors in that.

Do you tell me that result.errors (at the end of compile_restricted) contains (only) the real syntax errors collected by RestrictedPython?

If this is the case, why would you be unhappy?

In my previous comment, I told you how you would be able to distinguish SyntaxErrors generated by Python from the raised by compile_restricted.

What else do you need?

ScrambledRK commented 3 months ago

Thank you d-maurer for the insight and ideas.

Quite clever, but they all smell like a workaround to me. Maybe I'm just too stiff and conservative.

I don't think its a good idea to rebind or rewrite parts of a library that is so clearly related to security. I do not want to open any more opportunities for mistakes and risks than I already do using the library in the first place. Relying on the number of elements args contains seems bold to me. I rather not rely on something that only "appears" to be so. Dunno, I rather not differentiate then at all. At the cost of worse user feedback.

In my case I'd like to ... a) inform the user that what they are trying to do is usually okay, but not in my environment b) use it in tests to exclude accidental test passes due to actual syntax errors in the test code

Since I am just prototyping and experimenting for now, my need for this feature is very low. But the "accepting PR" is noted x)

loechel commented 3 months ago

@ScrambleRK and @d-maurer when I see it correctly the SyntaxError in https://github.com/zopefoundation/RestrictedPython/blob/master/src/RestrictedPython/compile.py#L211 is the only one that is raised by RestrictedPython itself. As it just lists all collected errors I wonder if SyntaxError is the correct Error or Exception for this use case.

As RestrictedPython is a restricted sub set of Python, everything that raises a real SyntaxError is really a Syntax Error found by the AST Module or the internal interpreter. All Restrictions should be collected in another way and should be announced in a different way.

If we change this one line of code and introduce a new Exception / Error Class that would be a breaking change and needs a new major version. @icemac any comments?

icemac commented 3 months ago

@loechel Changing the SyntaxError we raise to a subclass of SyntaxError would make the problem for the consumers of RestictedPython a bit less problematic as at least the isinstance tests still work. But I'd still consider this a major change.