Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I use the following functions to encrypt my $_GET variables (whenever I can't easily get away with using $_POST or some other way of passing information between pages.

 function decryptStringArray ($stringArray, $key = "Your secret salt thingie")
 {
    $s = unserialize(rtrim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, md5($key), base64_decode(strtr($stringArray, '-_,', '+/=')), MCRYPT_MODE_CBC, md5(md5($key))), "\0"));
    return $s;
 }

 function encryptStringArray ($stringArray, $key = "Your secret salt thingie") 
 {
    $s = strtr(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, md5($key), serialize($stringArray), MCRYPT_MODE_CBC, md5(md5($key)))), '+/=', '-_,');
    return $s;
 }

 function prepareUrl($url, $key = "Your secret salt thingie")
 {
    $url = explode("?",$url,2);
    if(sizeof($url) <= 1)
       return $url;
    else
       return $url[0]."?params=".encryptStringArray($url[1],$key);
 }

 function setGET($params,$key = "Your secret salt thingie") 
 {
    $params = decryptStringArray($params,$key);
    $param_pairs = explode('&',$params);
    foreach($param_pairs as $pair)
    {
       $split_pair = explode('=',$pair);
       $_GET[$split_pair[0]] = $split_pair[1];
    }
 }

Obviously I replace the "Your secret salt thingie" with other strings. Here is how I use it:

On the page where I need a url:

$url = prepareUrl("http://someurl.com?variable1=1314&variable2=1851&variable3=stringstuff", "algjalgjalgjal");

Then i put the new $url in a href or a tag or something ( i use $smarty templates but that isn't relevant)

On the page someurl.com where i need to decrypt the params i just use:

setGET($_GET['params'],"algjalgjalgjal"); 

Anyhow this all works fine for me. My only question is, is there anything inherently terrible about this way of doing things? My question is because I posted this as an answer on stack overflow to a question someone asked about hiding their $_GET parameters and it was immediately down-voted. That made me curious about whether it was somehow bad code or insecure in some way.

share|improve this question
3  
$_POST over HTTPS is the correct approach security and functionality wise. Encrypting GET variables is just reinventing the wheel, and what happens if you forget to encrypt a param? (I'm assuming you're hiding data from snoopers, not the actual user of the website, correct?) –  Corbin Jan 8 '13 at 2:25
    
actually i'm hiding it from the users too. –  nathan hayfield Jan 8 '13 at 18:43
    
Hiding it from the user tends to imply that you're either misusing GET or you're not fortifying your code enough. If you're using GET for passwords or something, don't; use POST instead. But if you're using it so that a user can't change admin=0 to admin=1 then there's a serious problem. –  Corbin Jan 9 '13 at 2:16
    
mostly its so they can't change some_record_id=120 to something else because certain users only have access to the records assigned to them, its definitely not for admin things. and its not mission critical. –  nathan hayfield Jan 9 '13 at 16:48
add comment

2 Answers

up vote 3 down vote accepted

Agree 100% with Corbin. GET isn't something that is inherently secure, and trying to make it so is nigh impossible. That's what POST is for. Only use GET for non-sensitive information. That being said, there are some generally concerning bits to your code that could cause some people to downvote it.

Your line length is rather long and convoluted, which causes major issues with legibility. I would suggest breaking up that single line into multiple lines to more easily maintain and read. Adding whitespace couldn't hurt either. Additionally, using single letter variables is not very descriptive and also leads to issues with legibility. Its a bit longer, but much easier to read.

$translation = strtr( $stringArray, '-_,', '+/=' );

$key     = md5( $key );
$data    = base64_decode( $translation );
$iv      = md5( $key );
$decrypt = mcrypt_decrypt(
    MCRYPT_RIJNDAEL_256,
    $key,
    $data,
    MCRYPT_MODE_CBC,
    $iv
);

$trimmed = rtrim( $decrypt, "\0" );

return unserialize( $trimmed );

A couple more potential issues with the above code are the methods you are implementing. I am by no means a security guru and have not put a lot of research into the matter, but I seem to remember from somewhere that mcrypt is frowned upon and that hashing a string with md5() twice is actually less secure than doing it just once. I don't know if this is true or not, but that could be a potential issue.

Your code is also slightly repetitive, violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat. Your encrypt and decrypt functions seem to share some common elements, using some shared helper functions to provide that similar data might be beneficial, even though you might just end up creating wrapper functions. I don't really see anyone downvoting you for this alone, this is rather minor in this instance and is rather hard to spot due to the above reasons.

Another potential issue I see is with your braceless syntax. Braceless {} syntax can be somewhat confusing to those who have never seen it before, and therefore could cause issues with maintainability. This is entirely a point of preference, but one I would argue most vehemently against. It is especially bad to offer in the form of an answer when it is unknown how the questioner will use it. It is quite possible they attempted to modify the code and could not get it to work.

The last issue I see is the way you are accessing array elements with magic numbers. This is rather sloppy and could cause issues with legibility, though I don't think you would get downvoted for it. There are a number of different ways this could be solved. The first is by using the PHP construct list(), which is probably preferable in this instance. The second is by using array functions to slice off the required portions of the array. This is more beneficial when you need the first and last elements of an array of undetermined length. Finally, there is also the possibility of using extract(), but that requires an associative array and is sometimes frowned upon. I won't show that last method because it doesn't apply here, but here are the other two:

//using list
list( $baseurl, $params ) = $url;

//using array functions
$baseurl = array_shift( $url );
$params  = array_shift( $url );//could potentially use array_pop()

return $baseurl . '?params=' . encryptStringArray( $params, $key );

Hope this helps!

share|improve this answer
    
thank you very much –  nathan hayfield Jan 8 '13 at 18:47
    
fyi the reason i am using $_GET is because i'm using it for popup windows from a main page to do sub forms and its the easiest way. I like encrypting it so the users don't see the names of the id's i'm sending. it doesn't really need to be all that secure but a little obscurity doesn't hurt because i have had some users that like to fool around with the urls (in the past) –  nathan hayfield Jan 8 '13 at 18:51
    
I'd take a look at AJAX, it sounds like what you really need. –  mseancole Jan 8 '13 at 18:54
1  
+1, but 2 nitpicks (both for the potential benefit of a future reader -- I suspect you're aware of both): list() should really only be used when you know for certain that the array is large enough. I suppose it could be a calling contract of his function though that a url must have a ? in it. –  Corbin Jan 9 '13 at 2:13
1  
Also, the statement about GET and POST is slightly misleading. The only security difference between get and post is that get shows up in URLs. Both are sent in clear text; both are easily altered, and both can be sniffed. The only difference is that URLs (and thus GET data) tend to be a lot more visible than POST data (history logs, company proxy logs, someone looking over your shoulder, etc). Anyway, </pedant-mode> :). –  Corbin Jan 9 '13 at 2:14
show 1 more comment

So far I think you are doing a decent job of obfuscation, if thats what you want.

If only your server is supposed to see the data, then it should stay on your server. Your server will have some index of data (session? database?) mechanism for identifying which data the client is working with, and only send the client the index to the data, not the data itself.

BUT based on your comment about wanting to obfuscate the indices, maybe some more validation logic on your server would be the best solution. If you don't want the user to mess with the query string to do something, then come up with rules that the server can use to determine whether access is valid at that time and allow the user to create his own query strings if he wants to.

You can't build a truly secure website until you can allow advanced users to access your server by any means they choose, including constructing raw HTTP requests.

share|improve this answer
    
its an intranet so it already has limited access, and you have to be logged in to view anything. raw http requests will only work if they are logged in with valid username password otherwise it will redirect to log in. there are some raw requests that we wouldn't want them to execute because they may not have the permissions to update that status or whatever the case may be. there is a heavy permissions based system on who can do what. –  nathan hayfield Jan 9 '13 at 16:52
1  
If only certain users should have access to certain items, and your system gives access to these items, you need to be looking up permissions in your system when a user accesses items. So if they change the ID in the URL, you can look up if they have access to that ID, and if not, don't let them do that operation. It might not seem like a big deal on your intranet, but then why do you have a policy about users access at all? Someone at your company cares about that policy and that it is followed. And what if one day you write an internet app? –  AndyClaw Jan 9 '13 at 18:26
    
we do recheck their permissions. the obscured url is just a further step so they cannot even see the records let alone do anything with them. –  nathan hayfield Jan 9 '13 at 22:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.