Skip to content

Commit

Permalink
Keep this function, as the one from the library appears to crash.
Browse files Browse the repository at this point in the history
  • Loading branch information
@dannybackx
dannybackx committed Nov 29, 2016
1 parent 848c973 commit d7f58e2
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 6 deletions.
9 changes: 7 additions & 2 deletions httpd/auth.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,20 @@ int ICACHE_FLASH_ATTR authBasic(HttpdConnData *connData) {
char userpass[AUTH_MAX_USER_LEN+AUTH_MAX_PASS_LEN+2];
char user[AUTH_MAX_USER_LEN];
char pass[AUTH_MAX_PASS_LEN];

// os_printf("authBasic()\n");
if (connData->conn==NULL) {
//Connection aborted. Clean up.
return HTTPD_CGI_DONE;
}

r=httpdGetHeader(connData, "Authorization", hdr, sizeof(hdr));
// os_printf("httpdGetHeader -> %d {%s}\n", r, hdr);
if (r && strncmp(hdr, "Basic", 5)==0) {
r=base64_decode(strlen(hdr)-6, hdr+6, sizeof(userpass), (unsigned char *)userpass);
r=my_base64_decode(strlen(hdr)-6, hdr+6, sizeof(userpass), (unsigned char *)userpass);
if (r<0) r=0; //just clean out string on decode error
userpass[r]=0; //zero-terminate user:pass string
// os_printf("Auth: %s\n", userpass);
// os_printf("Auth: %s\n", userpass);
while (((AuthGetUserPw)(connData->cgiArg))(connData, no,
user, AUTH_MAX_USER_LEN, pass, AUTH_MAX_PASS_LEN)) {
//Check user/pass against auth header
Expand All @@ -43,6 +46,7 @@ int ICACHE_FLASH_ATTR authBasic(HttpdConnData *connData) {
userpass[strlen(user)]==':' &&
os_strcmp(userpass+strlen(user)+1, pass)==0) {
//Authenticated. Yay!
os_printf("Authenticated as '%s', Yay !\n", user);
return HTTPD_CGI_AUTHENTICATED;
}
no++; //Not authenticated with this user/pass. Check next user/pass combo.
Expand All @@ -56,6 +60,7 @@ int ICACHE_FLASH_ATTR authBasic(HttpdConnData *connData) {
httpdEndHeaders(connData);
httpdSend(connData, forbidden, -1);
//Okay, all done.
os_printf("Not authenticated !\n");
return HTTPD_CGI_DONE;
}

4 changes: 2 additions & 2 deletions httpd/base64.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ static int ICACHE_FLASH_ATTR base64decode(const char in[4], char out[3]) {
#endif

/* decode a base64 string in one shot */
int ICACHE_FLASH_ATTR base64_decode(size_t in_len, const char *in, size_t out_len, unsigned char *out) {
int ICACHE_FLASH_ATTR my_base64_decode(size_t in_len, const char *in, size_t out_len, unsigned char *out) {
unsigned int ii, io;
uint32_t v;
unsigned int rem;
Expand Down Expand Up @@ -109,4 +109,4 @@ int base64_encode(size_t in_len, const unsigned char *in, size_t out_len, char *
return io;
}

#endif
#endif
4 changes: 2 additions & 2 deletions httpd/base64.h
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#ifndef BASE64_H
#define BASE64_H

int base64_decode(size_t in_len, const char *in, size_t out_len, unsigned char *out);
int my_base64_decode(size_t in_len, const char *in, size_t out_len, unsigned char *out);

#endif
#endif

10 comments on commit d7f58e2

@fuzzball03
Copy link
Owner

@fuzzball03 fuzzball03 commented on d7f58e2 Nov 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I wondered if we would need to do this. (is it me or does my_function make you think of writing your first code?)
I couldn't find the source for the ssl library - do you know where I can find it?

The esp library is a bit large, I'm hoping we can use a skeleton version of it to keep the build smaller.

By the way - how did it function? As expected?

@fuzzball03
Copy link
Owner

Choose a reason for hiding this comment

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

@dannybackx This might be a worthwhile read esp8266/Arduino#43
Doing this all from my cell right now, so taking some time to go through it all.

@fuzzball03
Copy link
Owner

Choose a reason for hiding this comment

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

Well looky here...
https://drive.google.com/file/d/0B9AEI0achpAGVDFwWkpXRnRDTlE/view?usp=drivesdk
This pdf should be more than enough.

Looks like memory constraints are going to be tough.
If it does not already, certificates will likely need to by streamed from filesystem and not stored in memory.

@dannybackx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know about the espressif docs. The SSL manual doesn't say much. If you look at example code, you'll see that you mostly need to replace espconn_xx calls by espconn_secure_xx calls.
I have code sitting on my disk for that, but it doesn't work. I will read the esp8266/Arduino#43 and will do some more tests, but for me this is not priority jeelabs#1.
I would categorise as jeelabs#1 to finish what we talked about yesterday, and jeelabs#2 talk this through with @tve so it gets picked up. Only jeelabs#3 is getting SSL to work.
Agree ?

@dannybackx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And yes, to get SSL support in order, storing keys/certificates in the filesystem is required.

Did it work ? To some extent, yes. I need to do more testing. Things weren't stable and I'm not sure why.

An obvious problem with the construction in main.c is that the (HTML/JavaScript) UI does some calls such as the JSON you referred to; securing these should not result in a UI that stops working.

@fuzzball03
Copy link
Owner

@fuzzball03 fuzzball03 commented on d7f58e2 Nov 29, 2016 via email •
edited
Loading

Choose a reason for hiding this comment

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

@fuzzball03
Copy link
Owner

@fuzzball03 fuzzball03 commented on d7f58e2 Nov 29, 2016 via email •
edited
Loading

Choose a reason for hiding this comment

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

@dannybackx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to investigate whether I am right on that paragraph. Tomorrow. It's after midnight now, just back from volleyball and off to bed... Don't know where you're located, I'm in Belgium.

  1. I would suggest offering the four options that we currently have in the menu; with one to be implemented.
    The old implementation is insecure, so offering to close those ports or to password-protect them is a
    bonus.
  2. We can if that makes the change more acceptable.
    Argument against that : the only conflicts are with questionable elements ("hacks") in the source code.
  3. I don't think I expressed this before, but I think we need to define which risks to address.
    Example : if you would state that communication between the ESP and Arduino need to be secure,
    then you're defending against someone with physical hardware access.
    This would be wrong : if someone already has physical access, then you have other problems.

To translate my example to your question : I think we need to assess which services in main.c to
(password-)protect and see if we can consistently implement that.
That's an order of magnitude more important (security wise) than hashing a password.

Just my analysis. Please comment.

@fuzzball03
Copy link
Owner

Choose a reason for hiding this comment

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

  1. I'm sorry, that's my fault for not being more clear.
    I meant to refer to username/pass required to visit the webpage.

  2. My only thought here is that tve appears to be a very detailed analyzer - I also know he mentioned reduction of RAM & other space limitations. So needlessly having it in the build only increases it's size.

  3. Yes, of course. My first approach to the security was trying to take into account physical access - but really, there's very little you can do to prevent someone cracking/extracting your code with physical access to the device. Once I got that line of thought out of my head, I tried to focus on remote access security
    With that in mind, I plan to:

  • Never return the password stored in flashConfig for Telnet - accessible only via internal functions. Though this would only obfuscate the password, perhaps storing it in base64 would be a good idea? After all, we already have the function available to us.
  • To expand on the above, the Telnet settings webpage will only display several asterisks if the password has been set, otherwise it will appear blank/empty.
  • Passwords will be passed from the webpage(ie we're setting/changing our password)in an obfuscated format, if anything just to make it more difficult for the casual user to identify. (anyone with access to the source can easily decode it)

As far as consistently implementing an 'authBasic' login into the webpage, this is quite easy to do via the main.c The rules are applied in a top->bottom scheme, so just placing a line similar to {"/telnet.html", authBasic, myPassFn},above the page your protecting is all it should take.

@dannybackx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm ok with your line of thought, including removal of ssl from Makefile for now.
So go ahead with changes.

Please sign in to comment.