Repository navigation
Security hardening across query builder, storage, auth, mail and crypto - #430
Merged
Merged
Conversation
- Database\QueryBuilder: bind whereBetween/whereNotBetween range values (SQL injection) and validate column/table identifiers in where*, join, select, aggregate, distinct, increment and insert/update keys through assertSafeIdentifier (wildcards allowed only where legitimate). - Storage\DiskFilesystemService::path(): confine resolved paths to the base directory and reject traversal (arbitrary read/write/delete). - Mail\Envelop: strip CR/LF/NUL from header-bound values (header/SMTP injection, CWE-93). - Middleware\CsrfMiddleware: reject empty tokens and compare with hash_equals (null==null bypass and timing). - Auth\SessionGuard + Session::regenerate(): rotate the session id on login while preserving data (session fixation). - Security\Crypto::decrypt(): fail closed on non-authenticated legacy (static-IV) input unless explicitly opted in via allowLegacy(). - Http: sanitize download filename; basename() the explicit upload target. Adds regression tests for the above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One commit fixing a set of security issues found across the framework, each with a regression test. No public API is removed; one opt-in method is added (
Crypto::allowLegacy()).Fixes
Database
QueryBuilder(SQL injection)whereBetween()/whereNotBetween()concatenated both range bounds into the SQL. They are now?placeholders bound throughwhere_data_binding.assertSafeIdentifier()inwhere,whereNull,whereNotNull,whereIn,whereNotIn,whereBetween,whereNotBetween,join/leftJoin/rightJoin(table and both sides of theon),select,distinct, aggregates,increment/decrement, and the keys ofinsertandupdate. Only a plain or table-qualified name is accepted.*andtable.*are allowed only forselect,distinctand aggregates.groupBy,havingandorderByalready used the check and now call it as an instance method.Storage
DiskFilesystemService::path()(path traversal)..segment is rejected before touching the disk, and the resolved path (or its parent for files that don't exist yet) must stay under the base directory, which also blocks symlink escapes. Violations throwResourceException.Mail
Envelop(header / SMTP injection, CWE-93)withHeader(),subject(),from(),addBcc(),addCc(),addReplyTo(),addReturnPath()andreturnPath()strip CR, LF and NUL from every header-bound value.CsrfMiddleware(bypass and timing)_tokenpath previously used loose comparison, so an unset session token (null) matched an absent token. Empty session tokens are now rejected and comparison useshash_equals().SessionGuard+Session::regenerate()(session fixation)attempts()success and onlogin().Session::regenerate()now relies onsession_regenerate_id(true)alone, which deletes the old record while preserving$_SESSION, instead of flushing and restarting, so the authenticated user survives the rotation.Crypto::decrypt()(unauthenticated ciphertext)BOW2:authenticated header now fails closed and returnsfalse. The legacy static-IV, no-MAC format can be re-enabled explicitly withCrypto::allowLegacy(true)while migrating old ciphertexts.HTTP
Response::download()strips directory components, CR, LF and quotes from theContent-Dispositionfilename and quotes it.UploadedFile::moveTo()appliesbasename()to an explicit target filename.Tests added
QueryBuilderTest: unsafewherecolumn is rejected;whereBetweenbinds both bounds (checked against a hostile upper bound).DiskFilesystemTest:path('../../x')throws;get('../secret')cannot read a file outside the base directory.SecurityTest:decrypt()fails closed on a legacy ciphertext unlessallowLegacy()is on.EnvelopHeaderInjectionTest: header-bound values with CR/LF cannot inject headers.Verification
Ran the regression tests listed above locally:
Behaviour changes to be aware of
where('1=1 ...')orselect('count(*) as total')) will now throwQueryBuilderException. Use a subqueryQueryBuilderfor expressions.Crypto::decrypt()returnsfalsefor pre-BOW2:ciphertexts unlessCrypto::allowLegacy(true)is called.DiskFilesystemServicemethods throwResourceExceptionfor paths outside the base directory instead of silently operating on them.