Summary: | ASTERISK-25857: func_aes: incorrect use of strlen() leads to data corruption | ||
Reporter: | Gianluca Merlo (gian) | Labels: | |
Date Opened: | 2016-03-18 20:30:47 | Date Closed: | 2016-03-22 09:05:04 |
Priority: | Major | Regression? | |
Status: | Closed/Complete | Components: | Functions/func_aes |
Versions: | 11.21.2 13.6.0 13.7.2 | Frequency of Occurrence | Constant |
Related Issues: | |||
Environment: | Attachments: | ||
Description: | Hello,
I tried to use {{AES_ENCRYPT}}/{{AES_DECRYPT}} from {{func_aes}} for reasons similar to the ones in issue leading to their addition (ASTERISK-13422). In my case, the integrity of data passing through was fortunately monitored by validating their format. On all versions I tried, I observed that some of the data seemed "corrupted". I took interest in the issue, and ran encryption/decription stress tests by placing a call to a dialplan snippet such as: {noformat} exten => _[a-zA-Z0-9].,1,Noop same => n,Set(KEY=abcdefghijklmnop) same => n,Ringing() same => n,Wait(5) same => n,Answer() same => n,Set(TIMEOUT(absolute)=120) same => n,Set(cycles=0) same => n,While($[ ${INC(cycles)} < 10000 ]) same => n,Set(original=${RAND()}:${RAND()}:${RAND()}:${RAND()}) same => n,Set(crypt=${AES_ENCRYPT(${KEY},${original})}) same => n,GotoIf($[ ${LEN(${crypt})} > 0 ]?decrypt) same => n,Log(NOTICE,TEST is NULL - orig <${original}> crypt <${crypt}> decrypt <>) same => n,ContinueWhile() same => n(decrypt),Set(decrypt=${AES_DECRYPT(${KEY},${crypt})}) same => n,GotoIf($[ "${original}" = "${decrypt}" ]?ok) same => n,Log(NOTICE,TEST is FAIL - orig <${original}> crypt <${crypt}> decrypt <${decrypt}>) same => n,ContinueWhile() same => n(ok),Log(NOTICE,TEST is GOOD - orig <${original}> crypt <${crypt}> decrypt <${decrypt}>) same => n,EndWhile() same => n,Hangup() {noformat} experiencing a consistent number of failures. Due to their nature, I looked at the code for {{func_aes}} and supposedly found the problem in the misuse of {{strlen}}. This is used to get the length of the data to encode in base64 in {{AES_ENCRYPT}}, but the data therein is binary, thus {{strlen}} will underestimate it at the first NULL character found: {code} ast_base64encode(buf, (unsigned char *) tmp, strlen(tmp), len); {code} A more correct estimation, IMHO, can be obtained by evaluating the offset reached by the write pointer {code} ast_base64encode(buf, (unsigned char *) tmp, tmpP - tmp, len); {code} code patched this way seems to work fine when stressed test with random data. I am submitting the patch for code review on Gerrit. As a fun fact, similar pitfalls were discussed in the original code review at https://reviewboard.asterisk.org/r/128/. | ||
Comments: | By: Asterisk Team (asteriskteam) 2016-03-18 20:30:47.621-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. |