Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash on invalid rails6-format cookie #1

Open
wants to merge 4 commits into
base: rails-6
Choose a base branch
from
Open

Fix crash on invalid rails6-format cookie #1

wants to merge 4 commits into from

Conversation

guisehn
Copy link

@guisehn guisehn commented Dec 13, 2024 •
edited
Loading

When providing an invalid cookie in Rails 6 format (with 3 parts), MessageEncryptor.authenticate_and_decrypt/2 could cause exceptions:

  1. if any of the three parts was invalid base64, it would raise an ArgumentError like:
Elixir.ArgumentError non-alphabet character found: " " (byte 32) 
    lib/base.ex:137 Base.bad_character!/1
    lib/base.ex:649 Base."-decode64base!/2-lbc$^0/2-0-"/2
    lib/base.ex:640 Base.decode64base!/2
    lib/enum.ex:1658 Enum."-map/2-lists^map/1-0-"/2
    lib/plug_rails_cookie_session_store/message_encryptor.ex:74 PlugRailsCookieSessionStore.MessageEncryptor.authenticate_and_decrypt/3
    lib/plug_rails_cookie_session_store.ex:99 PlugRailsCookieSessionStore.get/3
    lib/plug/session.ex:78 anonymous fn/5 in Plug.Session.fetch_session/1

or:

Elixir.ArgumentError non-alphabet character found: " " (byte 32) 
    lib/base.ex:137 Base.bad_character!/1
    lib/base.ex:649 Base."-decode64base!/2-lbc$^0/2-0-"/2
    lib/base.ex:640 Base.decode64base!/2
    lib/enum.ex:1658 Enum."-map/2-lists^map/1-0-"/2
    lib/plug_rails_cookie_session_store/message_encryptor.ex:74 PlugRailsCookieSessionStore.MessageEncryptor.authenticate_and_decrypt/3
    lib/plug_rails_cookie_session_store.ex:99 PlugRailsCookieSessionStore.get/3
    lib/plug/session.ex:78 anonymous fn/5 in Plug.Session.fetch_session/1
  1. if it was valid base64 but an invalid payload, it would return {:ok, :error} instead of {:error}, which would crash on:
Request: GET /
** (exit) an exception was raised:
    ** (ArgumentError) errors were found at the given arguments:

  * 1st argument: not an iodata term

        :erlang.iolist_to_binary(:error)
        (jason 1.3.0) lib/jason.ex:69: Jason.decode/2
        (plug_rails_cookie_session_store 2.0.0) lib/plug_rails_cookie_session_store.ex:171: PlugRailsCookieSessionStore.decode/2
        (plug 1.13.6) lib/plug/session.ex:78: anonymous fn/5 in Plug.Session.fetch_session/1
        (plug 1.13.6) lib/plug/debugger.ex:287: Plug.Debugger.maybe_fetch_session/1
        (plug 1.13.6) lib/plug/debugger.ex:187: Plug.Debugger.render/6
        (plug 1.13.6) lib/plug/debugger.ex:162: Plug.Debugger.__catch__/5

We fixed to return just :error in both cases, which gracefully falls back to an empty session.

guisehn
guisehn commented Dec 16, 2024
View reviewed changes
test/plug_rails_cookie_session_store_test/message_encryptor_test.exs
Comment on lines +66 to +90
padding = <<2, 2>>
assert decrypted == {:ok, data <> padding}
end

test "it uses only the first 32 bytes to authenticate and encrypt/decrypt" do
data = <<0, "helloworld", 0>>
padding = <<4, 4, 4, 4>>
encrypted = ME.encrypt_and_authenticate(<<0, "helloworld", 0>>, @large)

decrypted = ME.authenticate_and_decrypt(encrypted, @large)
assert decrypted == {:ok, data}
assert decrypted == {:ok, data <> padding}

decrypted = ME.authenticate_and_decrypt(encrypted, @right)
assert decrypted == {:ok, data}
assert decrypted == {:ok, data <> padding}

decrypted = ME.verify_and_decrypt(encrypted, @right, @right)
assert decrypted == :error

encrypted = ME.encrypt_and_authenticate(<<0, "helloworld", 0>>, @right)

decrypted = ME.authenticate_and_decrypt(encrypted, @large)
assert decrypted == {:ok, data}
assert decrypted == {:ok, data <> padding}

decrypted = ME.authenticate_and_decrypt(encrypted, @right)
assert decrypted == {:ok, data}
assert decrypted == {:ok, data <> padding}
Copy link
Author

@guisehn guisehn Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Those tests were failing, padding-removal code was removed on 541b0c5 but tests were not updated

@guisehn guisehn marked this pull request as ready for review December 16, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@guisehn