Everything

php

Security implications of parsing env variables in .ini

php.internals

Sergii Shymko

3 years ago

Hi internals team,

The discussion of extending the .ini env variable parsing capabilities with ability to specify defaults it occurred to me that the while feature of env variables may have undesirable security implications.

For instance, functions parse_ini_string() and parse_ini_file() do support the aforementioned env variables syntax, because the underlying code is reused. That means that these functions can potentially be exploited to read sensitive information!

For example:
AWS_SECRET_ACCESS_KEY=amazonWebServicesSecretAccessKeyExample1 php -r 'var_export(parse_ini_string("secret=\${AWS_SECRET_ACCESS_KEY}"));'
array (
'secret' => 'amazonWebServicesSecretAccessKeyExample1',
)

This only affects INI_SCANNER_NORMAL (the default). Should the mode argument be changed to disallow the env parsing by default? Perhaps another constant can be introduced to activate it, for example:
INI_SCANNER_NORMAL | INI_SCANNER_PARSE_ENV

This would be a BC breaking behavior but it's doubtful many people expected the env variable parsing syntax to extend to parse_ini_file/string() functions in the first place.

P.S.
My email client doesn't properly support the top-reply and/or quoting requirements of the mailing list. That's what made me disengage from the mailing list to not annoy anybody with butchered reply threads until I find time to migrate to another email client.

Here's a related tweet of mine:
https://twitter.com/SergiiShymko/status/1679598903925129222?s=20

Regards,
Sergii Shymko

David Gebler

3 years ago

On Thu, Jul 13, 2023 at 10:25 PM Sergii Shymko <sergey@shymko.net> wrote:

> For instance, functions parse_ini_string() and parse_ini_file() do support
> the aforementioned env variables syntax, because the underlying code is
> reused. That means that these functions can potentially be exploited to
> read sensitive information!
>
> For example:
> AWS_SECRET_ACCESS_KEY=amazonWebServicesSecretAccessKeyExample1 php -r
> 'var_export(parse_ini_string("secret=\${AWS_SECRET_ACCESS_KEY}"));'
> array (
>   'secret' => 'amazonWebServicesSecretAccessKeyExample1',
> )
>

If you find any way to exploit this, you've already breached enough to
have sufficient access to read the entire environment available to the PHP
user anyway (for example, you already had a way to inject arbitrary code
into a script which is eval'd or whatever...) in which case, why would you
care about parse_ini_string when you could just e.g. var_dump(getenv())?

Dusk

3 years ago

On Jul 13, 2023, at 18:34, David Gebler <davidgebler@gmail.com> wrote:

> If you find any way to exploit this, you've already breached enough to
> have sufficient access to read the entire environment available to the PHP
> user anyway

I think Sergii's concern is that an application might be using parse_ini_string() to transform user-provided string data into an array, and that it might not expect environment variables to be expanded in this context.

IMO, this is a valid concern, and:

1) Expansion of environment variables and php_ini settings needs to be mentioned more prominently in the documentation for parse_ini_string() and parse_ini_file(), with an explicit caution against using the functions on untrusted input.

2) These expansions should probably be disabled by INI_SCANNER_RAW; that flag already disables certain other types of value interpolation. (Oddly, it doesn't disable expansion of constants either; that might be worth revisiting as well.)

David Gebler

3 years ago

On Fri, Jul 14, 2023 at 3:08 AM Dusk <dusk@woofle.net> wrote:

> 2) These expansions should probably be disabled by INI_SCANNER_RAW; that
> flag already disables certain other types of value interpolation. (Oddly,
> it doesn't disable expansion of constants either; that might be worth
> revisiting as well.)

Environment variable parsing is already disabled by INI_SCANNER_RAW mode,
isn't it? Personally I don't think the default/normal mode should behave
differently. If you're passing untrusted input to parse_ini_string, you
should be sanitizing, white listing or using raw mode anyway really.

Dusk

3 years ago

On Jul 14, 2023, at 09:03, David Gebler <davidgebler@gmail.com> wrote:

> On Fri, Jul 14, 2023 at 3:08 AM Dusk <dusk@woofle.net> wrote:
>> 2) These expansions should probably be disabled by INI_SCANNER_RAW; that
>> flag already disables certain other types of value interpolation. (Oddly,
>> it doesn't disable expansion of constants either; that might be worth
>> revisiting as well.)
>
> Environment variable parsing is already disabled by INI_SCANNER_RAW mode,
> isn't it?

Oops! You're correct (and it does also disable constant expansion). I was passing the flag to $process_sections by mistake.

BohwaZ/PHP

3 years ago

This is a valid concern, as we are side-loading plugins in our
software, where plugin information is defined in a .ini file.

But somehow I thought that INI_SCANNER_TYPED was a bit like
INI_SCANNER_RAW but also able to handle null/true/false values.

Unfortunately it's not the case:

php > var_export(parse_ini_string("secret=\${AWS_SECRET_ACCESS_KEY}",
false, INI_SCANNER_TYPED)); array (
'secret' => '42',
)

If you want to be able to have false/true/null values without expanding
variables and constants you are out of luck.

Maybe we should have another constant to be able to disable both
variable and constant expansion when using parse_ini_* functions, for
example:

parse_ini_file('file.ini', false,
INI_SCANNER_TYPED & ~INI_SCANNER_EXPANSION);

Or something similar.

Tim Düsterhus

3 years ago

Hi

On 7/14/23 18:03, David Gebler wrote:

>> 2) These expansions should probably be disabled by INI_SCANNER_RAW; that
>> flag already disables certain other types of value interpolation. (Oddly,
>> it doesn't disable expansion of constants either; that might be worth
>> revisiting as well.)
>
>
> Environment variable parsing is already disabled by INI_SCANNER_RAW mode,
> isn't it? Personally I don't think the default/normal mode should behave
> differently. If you're passing untrusted input to parse_ini_string, you
> should be sanitizing, white listing or using raw mode anyway really.
>

Defaults matter. Developers should not need to provide
INI_SCANNER_PLEASE_DONT_PWN_ME to safely use a function.

Yes, the function is documented to behave "like php.ini's parsing", but
injecting potentially sensitive environment variables still violates the
principle of least surprise for me. Nothing about the function's
behavior or documentation indicates that it might be unsafe to use with
untrusted input data.

A short term improvement might be adding an explicit yellow warning to
the documentation page.

Best regards
Tim Düsterhus