Register and Login procedures in MySQL-CodePudding

I'm creating a database to host user credentials and other data with PHPMyAdmin 5.1.1, for a school project. For the Registration and Login I wanted to use Stored Procedures to ensure some degree of security instead of splitting the functions between MySQL and PHP like many other examples on the web. Not sure if it is actually a good implementation since I didn't found many other examples where the entire Login is made with MySQL.

I am using sha1 to hash the passwords concatenated with randomly generated salt.

My table is made like this:

CREATE TABLE `credentials` (
`Code` int(32) NOT NULL,
`Email` varchar(48) NOT NULL,
`Passwd` binary(40) NOT NULL,
`Salt` binary(20) NOT NULL,
`Date` datetime NOT NULL DEFAULT current_timestamp()
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

The two procedures I have wrote:

DELIMITER $$

CREATE DEFINER=`root`@`localhost` PROCEDURE `Register_new_user` (IN `mail` VARCHAR(48), IN 
`pass` VARCHAR(48))  NO SQL
BEGIN
    DECLARE `salt` BINARY(20);
    SET `salt`=SUBSTRING(MD5(RAND()), -24);
    INSERT INTO `credentials`(`Email`,`Passwd`, `Salt`) VALUES(`mail`, SHA1(CONCAT(`pass`,`salt`)),`salt`);
END$$


CREATE DEFINER=`root`@`localhost` PROCEDURE `Log_in` (IN `mail` VARCHAR(48), IN `pass` 
VARCHAR(48), OUT `responseMessage` VARCHAR(64), OUT `ID` INT(32))  NO SQL
BEGIN
   DECLARE `salt` BINARY(20);
   SET `ID`=NULL;

   IF (EXISTS(SELECT `Code` FROM `credentials` WHERE BINARY `Email`=`mail` ORDER BY `Code` DESC LIMIT 1))
   THEN
       SET `salt`=(SELECT `Salt` FROM `credentials` WHERE BINARY `Email`=`mail`);
       SET `ID`=(SELECT `Code` FROM `credentials` WHERE BINARY `Email`=`mail` AND BINARY `Passwd`=SHA1(CONCAT(`Pass`,`salt`)));
       IF(`ID` IS NULL) THEN
           SET `responseMessage`='wrong password';
       ELSE
           SET `responseMessage`='Success';
       END IF;

   ELSE
       SET `responseMessage`='Error';
   END IF;
END$$

DELIMETER;

I'm not sure why this is not working. Every time I test the Login procedure with the right credentials it always output "wrong password". If I test manually the comparison of the stored password with the sha1 output it looks to me identical, so I don't know why it fails.

 SET @p=(SELECT `salt` FROM `credentials` WHERE `Email`="AAAA");
 SELECT SHA1(CONCAT("banana",@p));
 SELECT `Passwd` FROM `credentials` WHERE `Email`="AAAA";

This test return for the first and second select:

b6830fcf435250097bfa6233a47fded36fb33f17
b6830fcf435250097bfa6233a47fded36fb33f17

Please, can anybody point out what am I doing wrong? Could it be the format or size of the binary I store? Or what else? Thanks.

CodePudding user response：

I am not going to give my opinions on whether this amounts to a good or bad implementation. There are plenty of good discussions on this topic on SO.

There are a number of reasons why this is not working and you should have received a number of error messages when trying your code.

What is your intent with Code int(32) - Integer display width is deprecated and an integer has a maximum unsigned value of 4,294,967,295 - 11.1.2 Integer Types (Exact Value)
I am not sure why you are using binary for Passwd and Salt as you are currently storing the hex string values. If you want to benefit from the more efficient storage you need to UNHEX the values before storing them. In this case your Passwd could be stored in BINARY(20) and Salt could be stored in BINARY(12). Have a look at 11.3.3 The BINARY and VARBINARY Types.
Both stored procs are declared as NO SQL when the first should be MODIFIES SQL DATA as it inserts data, and the second should be READS SQL DATA as it SELECTs data. This is not a big issue as MySQL treats these characteristics as advisory only.
You declare salt as BINARY(20) and then try to assign a 24 character string - SET salt = SUBSTRING(MD5(RAND()), -24);
In your table declaration Code is NOT NULL but your INSERT statement does not specify a value for Code.
Your use of email when searching for credentials suggests there should be a unique index.
Column names are not case sensitive so when you SET salt=(SELECT Salt FROM credentials WHERE BINARY Email=mail); you are SELECTing the local variable not the table column.

Here's a modified table declaration -

CREATE TABLE `credentials` (
    `Code` INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
    `Email` VARCHAR(48) NOT NULL,
    `Passwd` BINARY(40) NOT NULL,
    `Salt` BINARY(24) NOT NULL,
    `Date` DATETIME NOT NULL DEFAULT current_timestamp(),
    UNIQUE INDEX `UQ_credentials_email` (`Email`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

And the stored procedures -

DELIMITER $$

DROP PROCEDURE IF EXISTS `Register_new_user`$$
CREATE PROCEDURE `Register_new_user` (
    IN `mail` VARCHAR(48),
    IN `pass` VARCHAR(48)
)
MODIFIES SQL DATA
BEGIN
    DECLARE `_salt` BINARY(24);
    SET `_salt` = SUBSTRING(MD5(RAND()), -24);
    INSERT INTO `credentials`(`Email`, `Passwd`, `Salt`) VALUES (`mail`, SHA1(CONCAT(`pass`, `_salt`)), `_salt`);
END$$

DROP PROCEDURE IF EXISTS `Log_in`$$
CREATE PROCEDURE `Log_in` (
    IN `mail` VARCHAR(48),
    IN `pass` VARCHAR(48),
    OUT `responseMessage` VARCHAR(64),
    OUT `ID` INT
)
READS SQL DATA
BEGIN
   DECLARE `_salt` BINARY(24);
   SET `ID` = NULL;

   SELECT `Salt` INTO `_salt` FROM `credentials` WHERE `Email` = `mail`;
   IF (`_salt` IS NOT NULL)
   THEN
       SELECT `Code` INTO `ID` FROM `credentials` WHERE `Email` = `mail` AND `Passwd` = SHA1(CONCAT(`Pass`, `_salt`));
       IF(`ID` IS NULL) THEN
           SET `responseMessage` = 'wrong password';
       ELSE
           SET `responseMessage` = 'Success';
       END IF;

   ELSE
       SET `responseMessage`='Error';
   END IF;
END$$

DELIMITER ;

Or to make more efficient use of the BINARY storage -

CREATE TABLE `credentials` (
    `Code` INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
    `Email` VARCHAR(48) NOT NULL,
    `Passwd` BINARY(20) NOT NULL,
    `Salt` BINARY(12) NOT NULL,
    `Date` DATETIME NOT NULL DEFAULT current_timestamp(),
    UNIQUE INDEX `UQ_credentials_email` (`Email`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

and the stored procedures modified with HEX() and UNHEX() as required -

DELIMITER $$

DROP PROCEDURE IF EXISTS `Register_new_user`$$
CREATE PROCEDURE `Register_new_user` (
    IN `mail` VARCHAR(48),
    IN `pass` VARCHAR(48)
)
MODIFIES SQL DATA
BEGIN
    DECLARE `_salt` CHAR(24);
    SET `_salt` = SUBSTRING(MD5(RAND()), -24);
    INSERT INTO `credentials`(`Email`, `Passwd`, `Salt`) VALUES (`mail`, UNHEX(SHA1(CONCAT(`pass`, `_salt`))), UNHEX(`_salt`));
END$$

DROP PROCEDURE IF EXISTS `Log_in`$$
CREATE PROCEDURE `Log_in` (
    IN `mail` VARCHAR(48),
    IN `pass` VARCHAR(48),
    OUT `responseMessage` VARCHAR(64),
    OUT `ID` INT
)
READS SQL DATA
BEGIN
   DECLARE `_salt` CHAR(24);
   SET `ID` = NULL;

   SELECT LOWER(HEX(`Salt`)) INTO `_salt` FROM `credentials` WHERE `Email` = `mail`;
   IF (`_salt` IS NOT NULL)
   THEN
       SELECT `Code` INTO `ID` FROM `credentials` WHERE `Email` = `mail` AND `Passwd` = UNHEX(SHA1(CONCAT(`Pass`, `_salt`)));
       IF(`ID` IS NULL) THEN
           SET `responseMessage` = 'wrong password';
       ELSE
           SET `responseMessage` = 'Success';
       END IF;

   ELSE
       SET `responseMessage`='Error';
   END IF;
END$$

DELIMITER ;