HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Youtube OAuth security

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
youtubeoauthsecurity

Problem

I am working with Youtube OAuth for the first time and I am not that great when it comes to security. I got the following code and would like to hear if you guys can find any security issue:

$code = $_GET['code'];

    $url = 'https://accounts.google.com/o/oauth2/token';

    $params = array(
        "code" => $code,
        "client_id" => "XXX",
        "client_secret" => "YYY",
        "redirect_uri" => "URL",
        "grant_type" => "authorization_code"
    );

    $curl = curl_init($url);

    curl_setopt($curl, CURLOPT_POST, true);
    curl_setopt($curl, CURLOPT_POSTFIELDS, $params);
    curl_setopt($curl, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
    curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false);
    curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);  

    $json_response = curl_exec($curl);
    curl_close($curl);
    $authObj = json_decode($json_response);

    $accountInfo = get_data('https://www.googleapis.com/youtube/v3/channels?part=id&mine=true&access_token='.$authObj->access_token.'');

    $accountInfo = json_decode($accountInfo, true);

    $linkedAccount = $db->QueryFetchArrayAll("SELECT * FROM `youtube_accounts` WHERE `channel_id`='".$accountInfo['items'][0]['id']."'");
    $accountExists = $db->QueryFetchArrayAll("SELECT * FROM `youtube_accounts` WHERE `user_id`='".$data['id']."' AND `status` != 3");

Solution

Security

Curl Set POST

I don't think that there is a problem with passing user input to curl_setopt as post field (something like http header splitting is not possible, and it doesn't even seem possibly to add additional post fields, or to overwrite existing ones).

Important information via GET

It's not a good idea to send anything confidential via GET. GET requests can be cached, they can be stored in the browser history, they show up in server logs, and so on. Use POST instead.

SQL injection

$accountInfo isn't controlled by you, so you shouldn't trust it, the same probably goes for $data. Never put variable data directly into an SQL query, use prepared statements instead.

CURLOPT_SSL_VERIFYPEER

If you disable this setting, you weaken the security of SSL. Yes, you still encrypt even if CURLOPT_SSL_VERIFYPEER is false, but you would also accept the key of a man in the middle instead of the key from the server, so an attacker could still read the communication.

Here is the warning from the documentation:


WARNING: disabling verification of the certificate allows bad guys to
man-in-the-middle the communication without you knowing it. Disabling
verification makes the communication insecure. Just having encryption
on a transfer is not enough as you cannot be sure that you are
communicating with the correct end-point.

Context

StackExchange Code Review Q#88664, answer score: 2

Revisions (0)

No revisions yet.