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].