A collection of minor improvements. #36

mia · 2019-02-23T06:16:31+01:00

mia commented

2019-02-23 06:16:31 +01:00

First-time contributor

Including rewrite of JS (latest standard)

Some important suggestions:

1. Add 'action=' to FORM tag.
	<form method="POST" class="login">
	-> <form action="./login" method="POST" class="login">

2. Don't say "Password incorrect"
	This allows the attacker know whether the account is exist or not.
	Added in this pull request.

3. Don't show user-inputed value (such as password)
	Added in this pull request.

Including rewrite of JS (latest standard) Some important suggestions: ``` 1. Add 'action=' to FORM tag. <form method="POST" class="login"> -> <form action="./login" method="POST" class="login"> 2. Don't say "Password incorrect" This allows the attacker know whether the account is exist or not. Added in this pull request. 3. Don't show user-inputed value (such as password) Added in this pull request. ```

albino commented

2019-02-24 17:03:49 +01:00

Hello,

Firstly, thanks for your contribution! There are quite a lot of changes so I'll go through them one by one:

The updated javascript - thank you for cleaning up my code! This indeed seems like a neater solution. I'll test it at some point in the future and merge it if it works.
'required' attribute on forms - another useful improvement, thanks for that. Again, I will test and merge at a later date.
Capital letters on buttons - this was a design decision but perhaps it would be better to use 'regular' case in the markup and change it to lowercase where required using a stylesheet. This would require changes to the stylesheet(s) which I'm happy to implement in time.
Commenting out a section of the charter - unfortunately, it's not that simple. The charter reflects .cyb's standing within OpenNIC and therefore changes should probably go through OpenNIC.
Additional buttons on some of the pages - this is good, thanks.
Changes to the email template - the use of 'any more' here is actually correct. See here.
Not saying "password incorrect" (etc) - while developing cyberman initially I decided that the convenience of knowing that you have the wrong email address outweighs the potential security risks. Users are already protected providing they have a strong enough password.
Changes to language in login.tt - good, thanks.
Not echoing back the email address/don't show user-inputted value - I'm not sure why we need this. Is there a case for it improving security?
Changes to records/add.tt - these need to be accompanied by back-end code (e.g. validation rules to avoid an invalid zone). When I have time I'll implement this.

Finally, this PR includes too many commits with nondescript messages for me to accept it as-is. I'm happy to clean up & remove the parts I don't want, you will of course receive credit for the changes you made.

Thanks again
albino

Hello, Firstly, thanks for your contribution! There are quite a lot of changes so I'll go through them one by one: * The updated javascript - thank you for cleaning up my code! This indeed seems like a neater solution. I'll test it at some point in the future and merge it if it works. * 'required' attribute on forms - another useful improvement, thanks for that. Again, I will test and merge at a later date. * Capital letters on buttons - this was a design decision but perhaps it would be better to use 'regular' case in the markup and change it to lowercase where required using a stylesheet. This would require changes to the stylesheet(s) which I'm happy to implement in time. * Commenting out a section of the charter - unfortunately, it's not that simple. The charter reflects .cyb's standing within OpenNIC and therefore changes should probably go through OpenNIC. * Additional buttons on some of the pages - this is good, thanks. * Changes to the email template - the use of 'any more' here is actually correct. [See here.](https://www.grammarly.com/blog/anymore-vs-any-more/) * Not saying "password incorrect" (etc) - while developing cyberman initially I decided that the convenience of knowing that you have the wrong email address outweighs the potential security risks. Users are already protected providing they have a strong enough password. * Changes to language in `login.tt` - good, thanks. * Not echoing back the email address/don't show user-inputted value - I'm not sure why we need this. Is there a case for it improving security? * Changes to `records/add.tt` - these need to be accompanied by back-end code (e.g. validation rules to avoid an invalid zone). When I have time I'll implement this. Finally, this PR includes too many commits with nondescript messages for me to accept it as-is. I'm happy to clean up & remove the parts I don't want, you will of course receive credit for the changes you made. Thanks again albino

mia commented

2019-02-25 00:17:45 +01:00

First-time contributor

Not echoing back the email address/don't show user-inputted value - I'm not sure why we need this.
Is there a case for it improving security?

Yes, for security.

I'm happy to clean up & remove the parts I don't want

Yes, feel free to change the parts :)

By the way, I've found a bug which allows the attacker to input bogus email address.

Steps: (Firefox)

1. http://opennic.cyb/register
2. Right-click "Email address" input field, select "Inspect Element"
3. Click "Inspector" tab, change
	<input name="email" id="email" value="" type="email">
	to <input name="email" id="email" value="" type="">
4. Enter one of them to email address, password, and confirm password.
	test2@hello.null;test3@hello.null;test4@hello.null;test5@hello.null
	aaaaaaa<script>alert();</script>
5. [ register ]

Result:

	Your account has been created and a confirmation email sent to aaaaaaa<script>alert();</script>aa@aa.aa. Please confirm your email address, then log in here.
	Your account has been created and a confirmation email sent to test2@hello.null;test3@hello.null;test4@hello.null;test5@hello.null. Please confirm your email address, then log in here.

Expected:

	Trusting user-input is extremely dangerous.
	You need to verify input format on _server side_.

> Not echoing back the email address/don't show user-inputted value - I'm not sure why we need this. > Is there a case for it improving security? Yes, for security. > I'm happy to clean up & remove the parts I don't want Yes, feel free to change the parts :) By the way, I've found a bug which allows the attacker to input bogus email address. Steps: (Firefox) ``` 1. http://opennic.cyb/register 2. Right-click "Email address" input field, select "Inspect Element" 3. Click "Inspector" tab, change <input name="email" id="email" value="" type="email"> to <input name="email" id="email" value="" type=""> 4. Enter one of them to email address, password, and confirm password. test2@hello.null;test3@hello.null;test4@hello.null;test5@hello.null aaaaaaa<script>alert();</script> 5. [ register ] ``` Result: ``` Your account has been created and a confirmation email sent to aaaaaaa<script>alert();</script>aa@aa.aa. Please confirm your email address, then log in here. Your account has been created and a confirmation email sent to test2@hello.null;test3@hello.null;test4@hello.null;test5@hello.null. Please confirm your email address, then log in here. ``` Expected: ``` Trusting user-input is extremely dangerous. You need to verify input format on _server side_. ```

mia commented

2019-02-25 00:21:07 +01:00

First-time contributor

Suggestion

Server-side RegExp test for email

^([a-zA-Z0-9+.-]{1,255})\@([a-z0-9.-]{1,255})\.([a-z]{2,50})$


test;
example+alias@gmail.com
u@me.org

Suggestion ``` Server-side RegExp test for email ^([a-zA-Z0-9+.-]{1,255})\@([a-z0-9.-]{1,255})\.([a-z]{2,50})$ test; example+alias@gmail.com u@me.org ```

albino commented

2019-02-25 00:31:55 +01:00

Yes, for security.

What makes it more secure though?

Yes, feel free to change the parts :)

Thanks v much, I'll get round to it

By the way, I've found a bug which allows the attacker to input bogus email address.

Yes, but when echoed back it is passed through html_entity, so there is no XSS vector.

Server-side RegExp test for email

You cannot reliably and reasonably validate email-addresses with regex. The best way is to send a confirmation email, which we are doing already. If a user inputs an invalid address, the only result is the confirmation email failing to send and the account never being activated.

>Yes, for security. What makes it more secure though? >Yes, feel free to change the parts :) Thanks v much, I'll get round to it >By the way, I've found a bug which allows the attacker to input bogus email address. Yes, but when echoed back it is passed through `html_entity`, so there is no XSS vector. >Server-side RegExp test for email [You cannot reliably and reasonably validate email-addresses with regex.](https://stackoverflow.com/questions/201323/how-to-validate-an-email-address-using-a-regular-expression) The best way is to send a confirmation email, which we are doing already. If a user inputs an invalid address, the only result is the confirmation email failing to send and the account never being activated.

mia commented

2019-02-25 03:54:46 +01:00

First-time contributor

it is passed through html_entity, so there is no XSS vector

If you're using html_entity, it's okay.

But what about this? Attacker can use your registration form to send emails to multiple recipients.

victim1@mail;victim2@mail;...victim111111110@mail

Sent this as email to /register:
xxx@zzz.pro;yyy@zzz.pro;noreply@gmail.com

Result:
Your account has been created and a confirmation email sent to xxx@zzz.pro;yyy@zzz.pro;noreply@gmail.com. Please confirm your email address, then log in here.

And your email server delivered emails to _3_ recipients.

xxx@zzz.pro's inbox:

Subject:   	Confirm your email address
From:   	cybnic@uptime.party
Date:   	x
To:   	xxx@zzz.pro
yyy@zzz.pro
noreply@gmail.com
Priority:   	Normal

> it is passed through html_entity, so there is no XSS vector If you're using html_entity, it's okay. But what about this? Attacker can use your registration form to send emails to multiple recipients. victim1@mail;victim2@mail;...victim111111110@mail ``` Sent this as email to /register: xxx@zzz.pro;yyy@zzz.pro;noreply@gmail.com Result: Your account has been created and a confirmation email sent to xxx@zzz.pro;yyy@zzz.pro;noreply@gmail.com. Please confirm your email address, then log in here. And your email server delivered emails to _3_ recipients. xxx@zzz.pro's inbox: Subject: Confirm your email address From: cybnic@uptime.party Date: x To: xxx@zzz.pro yyy@zzz.pro noreply@gmail.com Priority: Normal ```

albino commented

2019-02-25 09:29:14 +01:00

Good find. Opened #38

This pull request can be merged automatically.

You are not authorized to merge this pull request.

You can also view command line instructions.