Skip to content

Commit

Permalink
change apikey and salt generation to use new php7 cryptographically s…
Browse files Browse the repository at this point in the history
…ecure random_bytes function
  • Loading branch information
TrystanLea committed Jul 15, 2021
1 parent a8dc51e commit 31523b9
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 8 deletions.
12 changes: 12 additions & 0 deletions Modules/user/profile/profile.js
Expand Up @@ -124,6 +124,10 @@ var app = new Vue({
$.ajax({type:"POST",url: path+"user/deleteall.json", data: "mode=dryrun", dataType: 'text', success: function(result){
$("#deleteall-output").html(result);
}});
},
new_apikey: function(type) {
$("#apikey_type").html(type);
$('#modalNewApikey').modal('show');
}
}
});
Expand Down Expand Up @@ -161,6 +165,14 @@ $("#logoutdelete").click(function() {
}});
});

$("#confirm_generate_apikey").click(function() {
var type = $("#apikey_type").html();
$.ajax({ url: path+"user/newapikey"+type+".json", dataType: 'json', success: function(result){
app.user['apikey_'+type] = result;
$('#modalNewApikey').modal('hide');
}});
});

// Theme selection used in conjunction with code in Lib/emoncms.js
$(".themecolor[name='"+current_themecolor+"']").addClass("color-box-active");
$(".themecolor").click(function() {
Expand Down
23 changes: 22 additions & 1 deletion Modules/user/profile/profile.php
Expand Up @@ -25,6 +25,7 @@
<tr>
<td class="muted"><?php echo _('User ID'); ?></td>
<td>{{ user.id }}</td>
<td></td>
<td><button class="btn btn-small btn-danger" @click="delete_account()"><?php echo _('Delete account'); ?></button></td>
</tr>
<tr>
Expand All @@ -37,6 +38,7 @@
</div>
</td>
<td><i class="icon-pencil" v-if="!edit.username" @click="show_edit('username')"></i></td>
<td></td>
</tr>
<tr>
<td class="muted"><?php echo _('Email'); ?></td>
Expand All @@ -48,16 +50,19 @@
</div>
</td>
<td><i class="icon-pencil" v-if="!edit.email" @click="show_edit('email')"></i></td>
<td></td>
</tr>
<tr>
<td class="muted"><?php echo _('Write API Key'); ?></td>
<td><div class="apikey">{{ user.apikey_write }}</div></td>
<td><i class="icon-share" @click="copy_text_to_clipboard(user.apikey_write,'<?php echo _("Write API Key copied to clipboard"); ?>')"></i></td>
<td><button class="btn btn-small" @click="new_apikey('write')">Generate New</button></td>
</tr>
<tr>
<td class="muted"><?php echo _('Read API Key'); ?></td>
<td><div class="apikey">{{ user.apikey_read }}</div></td>
<td><i class="icon-share" @click="copy_text_to_clipboard(user.apikey_read,'<?php echo _("Read API Key copied to clipboard"); ?>')"></i></td>
<td><button class="btn btn-small" @click="new_apikey('read')">Generate New</button></td>
</tr>
<tr>
<td class="muted"><?php echo _('Password'); ?></td>
Expand All @@ -81,6 +86,7 @@
</div>
</td>
<td><i class="icon-pencil" v-if="!edit.password" @click="show_edit('password')"></i></td>
<td></td>
</tr>
</table>

Expand Down Expand Up @@ -231,8 +237,23 @@
</div>
</div>

<div id="modalNewApikey" class="modal hide" tabindex="-1" role="dialog" aria-labelledby="modalNewApikeyLabel" aria-hidden="true" data-backdrop="false">
<div class="modal-header">
<button type="button" class="close" data-dismiss="modal" aria-hidden="true">×</button>
<h3 id="modalNewApikeyLabel">Generate a new <span id="apikey_type"></span> API key</h3>
</div>
<div class="modal-body">
<p><?php echo _('Are you sure you want to generate a new apikey?'); ?></p>
<p><?php echo _("All devices using the current key will need to be updated with the new key."); ?></p>
</div>
<div class="modal-footer">
<button id="cancel_generate_apikey" class="btn" data-dismiss="modal" aria-hidden="true"><?php echo _('Cancel'); ?></button>
<button id="confirm_generate_apikey" class="btn btn-primary"><?php echo _('Generate'); ?></button>
</div>
</div>

<script>
var languages = <?php echo json_encode(get_available_languages_with_names()); ?>;
var str_passwords_do_not_match = "<?php echo _('Passwords do not match'); ?>";
</script>
<script type="text/javascript" src="<?php echo $path; ?>Modules/user/profile/profile.js?v=<?php echo $v; ?>"></script>
<script type="text/javascript" src="<?php echo $path; ?>Modules/user/profile/profile.js?v=<?php echo $v; ?>"></script> <p><?php echo _('Are you sure you want to generate a new apikey?'); ?></p>
2 changes: 1 addition & 1 deletion Modules/user/rememberme_model.php
Expand Up @@ -203,7 +203,7 @@ public function loginTokenWasInvalid() {
// Create a pseudo-random token.
// ---------------------------------------------------------------------------------------------------------
private function createToken() {
return md5(uniqid(mt_rand(), true));
return bin2hex(random_bytes(16));
}
// ---------------------------------------------------------------------------------------------------------
private function getCookieValues()
Expand Down
12 changes: 6 additions & 6 deletions Modules/user/user_model.php
Expand Up @@ -247,11 +247,11 @@ public function register($username, $password, $email, $timezone)
// If we got here the username, password and email should all be valid

$hash = hash('sha256', $password);
$salt = md5(uniqid(mt_rand(), true));
$salt = bin2hex(random_bytes(16));
$password = hash('sha256', $salt . $hash);

$apikey_write = md5(uniqid(mt_rand(), true));
$apikey_read = md5(uniqid(mt_rand(), true));
$apikey_write = bin2hex(random_bytes(16));
$apikey_read = bin2hex(random_bytes(16));

$stmt = $this->mysqli->prepare("INSERT INTO users ( username, password, email, salt ,apikey_read, apikey_write, timezone, admin) VALUES (?,?,?,?,?,?,?,0)");
$stmt->bind_param("sssssss", $username, $password, $email, $salt, $apikey_read, $apikey_write, $timezone);
Expand Down Expand Up @@ -297,7 +297,7 @@ public function send_verification_email($username)
if ($email_verified) return array('success'=>false, 'message'=>_("Email already verified"));

// Create new verification key
$verification_key = md5(uniqid(mt_rand(), true));
$verification_key = bin2hex(random_bytes(16));
// Save new verification key
$stmt = $this->mysqli->prepare("UPDATE users SET verification_key=? WHERE id=?");
$stmt->bind_param("si",$verification_key,$id);
Expand Down Expand Up @@ -812,7 +812,7 @@ public function set($userid,$data)
public function new_apikey_read($userid)
{
$userid = (int) $userid;
$apikey = md5(uniqid(mt_rand(), true));
$apikey = bin2hex(random_bytes(16));

$stmt = $this->mysqli->prepare("UPDATE users SET apikey_read = ? WHERE id = ?");
$stmt->bind_param("si", $apikey, $userid);
Expand All @@ -826,7 +826,7 @@ public function new_apikey_read($userid)
public function new_apikey_write($userid)
{
$userid = (int) $userid;
$apikey = md5(uniqid(mt_rand(), true));
$apikey = bin2hex(random_bytes(16));

$stmt = $this->mysqli->prepare("UPDATE users SET apikey_write = ? WHERE id = ?");
$stmt->bind_param("si", $apikey, $userid);
Expand Down

6 comments on commit 31523b9

@inverse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the min version is now PHP 7? so we can use scalar types?

@TrystanLea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @inverse good question, perhaps I need to abstract this function and check that it exists and if not provide a fallback? or use openssl_random_pseudo_bytes, do you have any view on the matter?

@TrystanLea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could alternatively use: $apikey = bin2hex(openssl_random_pseudo_bytes(16)); which is supported by 5.3 and has apparently had an earlier flaw fixed.

@TrystanLea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to create a function in core.php that abstracts out the key generation, using random_bytes if available and if not the openssl function 03ba19f
This should keep compatibility with PHP 5.3 for now

@TrystanLea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to consider whether we aim to target only PHP 7 as a separate matter. Latest emonSD images are now all PHP7 and all of the emoncms server's that I run, but I know it can be a real pain if you have an old server and pulling in the latest emoncms breaks everything!

@inverse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tough one since even PHP 7.2 is EOL since last year. Which supporting such things which are potentially exposed to the internet could be deemed a security risk.

https://endoflife.software/programming-languages/server-side-scripting/php

Check out WordPress and their proposal on it https://make.wordpress.org/core/2020/08/24/proposal-dropping-support-for-old-php-versions-via-a-fixed-schedule/

Please sign in to comment.