RestrictedPython icon indicating copy to clipboard operation
RestrictedPython copied to clipboard

separating security messages

Open jsmith173 opened this issue 1 year ago • 10 comments

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?

jsmith173 avatar Mar 26 '24 12:03 jsmith173

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 avatar Mar 28 '24 06:03 icemac

A PR is welcome.

icemac avatar Mar 28 '24 06:03 icemac

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.

d-maurer avatar Mar 28 '24 07:03 d-maurer

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'

jsmith173 avatar Mar 28 '24 08:03 jsmith173

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:

  • You can use your own compile_restricted which uses a different exception

  • You can use dm.reuse.rebind_function to bind the SyntaxError reference in compile_restricted to an exception of your choice.

  • You can look at the args attribute of the SyntaxError. When the SyntaxError was raised by Python, args appears to have more than a single element and the first one is a string; when compile_restricted raises SyntaxError, it is called with result.errors which gives a single element in args almost surely of type list.

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.

d-maurer avatar Mar 28 '24 08:03 d-maurer

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

jsmith173 avatar Mar 28 '24 10:03 jsmith173

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?

d-maurer avatar Mar 28 '24 11:03 d-maurer

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)

ScrambledRK avatar Jun 10 '24 14:06 ScrambledRK

@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?

loechel avatar Jun 10 '24 15:06 loechel

@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.

icemac avatar Jun 11 '24 06:06 icemac