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
For instance, functions
parse_ini_string()andparse_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())?
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:
-
Expansion of environment variables and php_ini settings needs to be mentioned more prominently in the documentation for
parse_ini_string()andparse_ini_file(), with an explicit caution against using the functions on untrusted input. -
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.)
- 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.
- 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_RAWmode,
isn't it?
Oops! You're correct (and it does also disable constant expansion). I was passing the flag to $process_sections by mistake.
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.
Hi
- 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_RAWmode,
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