[asterisk-dev] [Code Review] added AES_ENCRYPT and AES_DECRYPT dialplan functions

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/128/#review341
-----------------------------------------------------------



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment794>

    Minor nitpick, but you misspelled "encryptedtext" in both comments here.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment798>

    There is a potential for a buffer overflow here. This memcpy should only copy at most len bytes into tmp.
    
    If args.data is longer than buf's capacity, then you will overflow the buffer here.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment799>

    I agree with Tilghman's earlier comment of making this read if (!encrypt) instead of if (encrypt == 0) since it is being used as a boolean value.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment795>

    This line worries me a bit, in particular the third argument. The third argument used here is the capacity of the output buffer, and it seems that what is supposed to be used is the length of the input string.
    
    It looks like what will happen is that we will do a base64encode operation on not only the aes-encrypted string, but potentially on junk memory in tmp beyond the length of the encrypted string.
    
    This would not be a problem if we knew that buf was zeroed out completely before calling this function. Unfortunately, there are several places in the code which set the first byte of buf to 0 but leave the rest uninitialized before calling this code.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment796>

    Unlike in my last comment, the extra length being copied here is not a problem since tmp was allocated using calloc, which means the unused portions beyond the end of the string will be all 0's.



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/128/#comment797>

    If you allocate something using ast_calloc, then you also should free the memory using ast_free.


- Mark


On 2009-01-27 14:40:43, dvossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/128/
> -----------------------------------------------------------
> 
> (Updated 2009-01-27 14:40:43)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Mark Michelson.
> 
> 
> Summary
> -------
> 
> Allows data to be encrypted and decrypted using AES in the dialplan.  
> 
> 
> This addresses bug 0014301.
>     http://bugs.digium.com/view.php?id=0014301
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_aes.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/128/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> dvossel
> 
>