Skip to content

Conversation

jasinner
Copy link
Contributor

@jasinner jasinner commented Sep 19, 2024

Uses the python string format to demonstrate the Format Strings and Templates section of course.

This example came from Be Careful with Python's New-Style String Format by Armin Ronacher and was produced in collaboration with @david-a-wheeler

@jasinner
Copy link
Contributor Author

I'm having trouble getting the hints to work. Appreciate some help with that @david-a-wheeler

<pre><code
> # Application configuration which should be kept secret from a user
CONFIG = {
'SECRET_KEY': 'super secret key'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do that! That would be a secret key embedded in source code, which is itself a vulnerability.

the <a href="https://docs.python.org/3/tutorial/inputoutput.html#the-string-format-method"><tt>
format string</tt></a>. If a user can control the <tt>format string</tt> they can access
variables which they shouldn't. Particularly if those variable's values can be returned to the user
as output, it could lead to information disclosure beyond what was intended by the developer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add something like this:

In some cases the results can be even more serious, depending on the situation and programming language used. If a similar problem happened in C's printf function, it would often enable an attacker to completely control the program.

<p>
You could adjust the program so that it only formats the event, and does not include any user input,
However it is considered safe to include user input in the output as long as they cannot control
the <tt>format string</tt> itself.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's WAY too strong a claim. Just say that allowing arbitrary user control of format strings is dangerous.

Adjust the value returned by the <tt>format_event</tt> function so the the user controlled
<tt>user_input</tt> variable is only used as a
<a href="https://docs.python.org/3/library/string.html#format-string-syntax"><tt>replacement field</tt></a>
and is not used as the <tt>format string</tt>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? This doesn't look secure.

This still allows for user control of the field to be shown. Depending on the object being passed to format, that can still leak secret data that isn't supposed to be shared. It might be okay in context, but I think the lab should focus on the "key point" which is that format strings can be dangerous, and if you don't HAVE to have them controlled by the user, don't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Be Careful with Python's New-Style String Format article uses "the somewhat undocumented internals to change the behavior" in an attempt to sandbox things. That's a complex and dangerous way to resolve security issues. Sure, sometimes you must do something quirky like that, but there's a simple one: don't do it. If you only get format strings from trusted sources, the problem disappears. The article mentions translators, but usually translators are a small number of trusted people, very different from accepting arbitrary inputs from attackers.



<script id="expected0" type="plain/text">
return '{format_string}, {event.level}'.format(format_string=user_input, event=event)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still allows untrusted user input as a format string, and that's what we are telling them to avoid. Also, while "event=event" is Pythonic, it's really unclear to non-experts in Python, because they don't know which "event" is being used in the format string.

I'd expect something like this as an answer:

 return 'Event level = {event_level}'.format(event_level=event.level)


<script id="info" type="application/yaml">
---
- absent: "user_input"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing this line after ---:

hints:

We support several different keys in the info, "hints" is one of them.

Once you do, you'll find a lot of the tests fail. You'll need the tests to pass :-).

@david-a-wheeler
Copy link
Contributor

Thanks so much for the update! I provided a few comments. Can you please do a DCO signoff?

@david-a-wheeler
Copy link
Contributor

Hi - you're so close! Can you please finish this soon? Thanks so much!

@david-a-wheeler
Copy link
Contributor

I'm having trouble getting the hints to work. Appreciate some help with that @david-a-wheeler

You're missing the hints: field identifier. You should see something like this:

<script id="info" type="application/yaml">
---
hints:
- absent: "user_input"

@david-a-wheeler
Copy link
Contributor

Can you try to wrap this up sooner if you can? We're trying to get a number of the labs out-the-door :-).

Also: I had assigned "Debug and Assertion" to you as well. I thinking of unassigning until you complete this one, and once you're done with this, let me know which available ones you'd like to do. Pre-assigning more than one was probably a bad idea, sorry about that.

@david-a-wheeler
Copy link
Contributor

Hi, I haven't heard from you in 2 weeks, and we need to get moving. I do appreciate the work you've done so far. I plan to complete your lab myself and merge it, and do the debug lab myself instead. Thanks!

@david-a-wheeler
Copy link
Contributor

More needs fixing, but it's easier to merge first & create separate fixes.

@david-a-wheeler david-a-wheeler merged commit 74fae71 into ossf:main Oct 14, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants