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.

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

AEAD payload length check #2591

Merged

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Dec 22, 2022

If AES-GCM is enabled, 16 bytes of the payload are occupied for the authentication tag.
Hence the payload size must be properly validated when submitted to the SND buffer.

Once a connection is established and the MTU value is negotiated (minimum of the two), this value is used to determine the maximum allowed payload size. Thus any payload exceeding the value is not allowed to be sent.

Also fixes #2589.

Also fixes CSndBuffer::addBufferFromFile(..): wrong usage of iPktLen.

Example Test Case 01

 ./srt-xtransmit.exe generate "srt://127.0.0.1:4200?cryptomode=2&passphrase=abcdefghijk&payloadsize=1440" --sendrate 1Mbps --msgsize 1440 --enable-metrics -v

./srt-xtransmit.exe receive "srt://:4200?cryptomode=2&passphrase=abcdefghijk" --enable-metrics --msgsize 1456 -v

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Dec 22, 2022
@maxsharabayko maxsharabayko added this to the v1.5.2 milestone Dec 22, 2022
@maxsharabayko maxsharabayko changed the title Develop/aead payload check AEAD payload length check Dec 22, 2022
srtcore/buffer_snd.cpp Outdated Show resolved Hide resolved
srtcore/buffer_snd.cpp Outdated Show resolved Hide resolved
@maxsharabayko
Copy link
Collaborator Author

A question is what to do with the SRTO_PAYLOADSIZE max value. The SRTO_CRYPTOMODE is set to auto by default, thus allowing GCM connections. Then 1456 is no longer allowed unless an AES-CTR connection is established.

[ RUN      ] TestSocketOptions.MaxVals
15:10:51.963229/test-srt*E:SRT.sm: SRTO_GROUPMINSTABLETIMEO set 5000
Skipping SRTO_KMPREANNOUNCE
Skipping SRTO_KMREFRESHRATE
15:10:51.963401/test-srt*E:SRT.ac: SRTO_PAYLOADSIZE: value exceeds 1456 bytes decreased by 16 required for AES-GCM.
/home/runner/work/srt/srt/test/test_socket_options.cpp:264: Failure
Expected equality of these values:
  srt_setsockopt(sock, 0, entry.optid, &opt_val, opt_len)
    Which is: -1
  expect_return
    Which is: 0
Setting SRTO_PAYLOADSIZE to 1456 must succeed
/home/runner/work/srt/srt/test/test_socket_options.cpp:242: Failure
Expected equality of these values:
  opt_val
    Which is: 1316
  value
    Which is: 1456
[Caller, max value]: Wrong SRTO_PAYLOADSIZE value 1316
[  FAILED  ] TestSocketOptions.MaxVals (1 ms)

@maxsharabayko maxsharabayko marked this pull request as ready for review December 23, 2022 15:17
@ethouris
Copy link
Collaborator

There was generally a controversy around the SRTO_PAYLOADSIZE option. This option had to actually set the maximum size of the payload mainly for stats, but then it's useful for setting the maximum size for a single call to srt_send, and the minimum buffer size for srt_recv. There are two problems with it though:

  1. This value isn't negotiated. You can set this to 1456 on the sender side and the receiver side has still 1316 and is allowed to pass a too-small buffer.
  2. The feature to enforce the buffer size is desired, but SRTO_PAYLOADSIZE is for the single packet only.

Therefore I think it should be possible to kinda repurpose this option for something else, and this way:

  • In Live mode, it is always limited to 1456, minus possibly reserved space for FEC or AEAD
  • In Message mode, it is theoretically unlimited, but shall not exceed the flight flag size and sender buffer size
  • In Stream mode, it is always 0 and cannot be modified.

@ethouris
Copy link
Collaborator

BTW test_socket_options.cpp should be fixed accordingly.

@codecov-commenter
Copy link

Codecov Report

Merging #2591 (9c35a6c) into master (98b1b00) will increase coverage by 0.01%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##           master    #2591      +/-   ##
==========================================
+ Coverage   66.35%   66.36%   +0.01%     
==========================================
  Files          99       99              
  Lines       19822    19826       +4     
==========================================
+ Hits        13153    13158       +5     
+ Misses       6669     6668       -1     
Impacted Files Coverage Δ
srtcore/buffer_snd.h 50.00% <ø> (ø)
srtcore/strerror_defs.cpp 75.00% <ø> (ø)
srtcore/socketconfig.cpp 69.49% <50.00%> (-0.51%) ⬇️
srtcore/core.cpp 60.96% <60.00%> (+0.42%) ⬆️
srtcore/buffer_snd.cpp 68.70% <70.00%> (+0.95%) ⬆️
srtcore/api.cpp 53.28% <0.00%> (-1.76%) ⬇️
srtcore/group.cpp 37.76% <0.00%> (+0.05%) ⬆️
srtcore/list.cpp 81.70% <0.00%> (+0.25%) ⬆️
srtcore/utilities.h 63.63% <0.00%> (+0.44%) ⬆️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@maxsharabayko maxsharabayko merged commit 5889a2c into Haivision:master Jan 2, 2023
@maxsharabayko maxsharabayko deleted the develop/aead-payload-check branch January 2, 2023 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] SRTO_MSS = 1300 splits one message into two packets in TSBPD mode.
3 participants