Wrong output for substitution code in Python

Question

    def OnButtonClick(self):
        self.labelVariable.set("Encrypting...")
        string = self.entryVariable.get()
        encrypted_text = ""
        crypt = ""
        for c in string:
            crypt = crypt + c.replace('A','F5K')
            crypt = crypt + c.replace('B','O7C')
            crypt = crypt + c.replace('C','E1S')
            crypt = crypt + c.replace('D','K1M')
            crypt = crypt + c.replace('E','U5E')
            crypt = crypt + c.replace('F','Q2D')
            crypt = crypt + c.replace('G','J0J')
            crypt = crypt + c.replace('H','I3L')
            crypt = crypt + c.replace('I','T2T')
            crypt = crypt + c.replace('J','F7D')
            crypt = crypt + c.replace('K','R9Q')
            crypt = crypt + c.replace('L','L2Y')
            crypt = crypt + c.replace('M','G9C')
            crypt = crypt + c.replace('N','S3C')
            crypt = crypt + c.replace('O','F2E')
            crypt = crypt + c.replace('P','E3G')
            crypt = crypt + c.replace('Q','I0G')
            crypt = crypt + c.replace('R','O1W')
            crypt = crypt + c.replace('S','Q3G')
            crypt = crypt + c.replace('T','D0N')
            crypt = crypt + c.replace('U','S8I')
            crypt = crypt + c.replace('V','U8Y')
            crypt = crypt + c.replace('W','L4D')
            crypt = crypt + c.replace('X','B2K')
            crypt = crypt + c.replace('Y','L7K')
            crypt = crypt + c.replace('Z','W6L')
            crypt = crypt + c.replace('a','P8Q')
            crypt = crypt + c.replace('b','I5E')
            crypt = crypt + c.replace('c','C6D')
            crypt = crypt + c.replace('d','Q2L')
            crypt = crypt + c.replace('e','N4Y')
            crypt = crypt + c.replace('f','J8S')
            crypt = crypt + c.replace('g','D9I')
            crypt = crypt + c.replace('h','P2N')
            crypt = crypt + c.replace('i','A8D')
            crypt = crypt + c.replace('j','I2I')
            crypt = crypt + c.replace('k','W4Y')
            crypt = crypt + c.replace('l','V1B')
            crypt = crypt + c.replace('m','F4R')
            crypt = crypt + c.replace('n','Y7K')
            crypt = crypt + c.replace('o','L2K')
            crypt = crypt + c.replace('p','J3J')
            crypt = crypt + c.replace('q','R2W')
            crypt = crypt + c.replace('r','A7S')
            crypt = crypt + c.replace('s','C9C')
            crypt = crypt + c.replace('t','S2M')
            crypt = crypt + c.replace('u','F4W')
            crypt = crypt + c.replace('v','V7F')
            crypt = crypt + c.replace('w','D6V')
            crypt = crypt + c.replace('x','A7L')
            crypt = crypt + c.replace('y','O5R')
            crypt = crypt + c.replace('z','Q3L')
            crypt = crypt + c.replace('1','Z3W')
            crypt = crypt + c.replace('2','B51')
            crypt = crypt + c.replace('3','G1N')
            crypt = crypt + c.replace('4','A8T')
            crypt = crypt + c.replace('5','N7T')
            crypt = crypt + c.replace('6','O7N')
            crypt = crypt + c.replace('7','J0L')
            crypt = crypt + c.replace('8','C7Q')
            crypt = crypt + c.replace('9','M7V')
            crypt = crypt + c.replace('0','N3B')
            crypt = crypt + c.replace(',','M5Z')
            crypt = crypt + c.replace('.','O2O')
            crypt = crypt + c.replace('!','E7L')
            crypt = crypt + c.replace('?','Z1N')
            crypt = crypt + c.replace("'",'G7M')
            crypt = crypt + c.replace('"','P6B')
            crypt = crypt + c.replace(' ','S31')
            crypt = crypt + c.replace('(','O4F')
            crypt = crypt + c.replace(')','H6F')
            crypt = crypt + c.replace('@','W9C')
            crypt = crypt + c.replace('#','B7Q')
            crypt = crypt + c.replace('&','G1Y')
            crypt = crypt + c.replace('*','Y2Z')
            crypt = crypt + c.replace('-','Z1X')
            crypt = crypt + c.replace('_','C9G')
            crypt = crypt + c.replace('+','O2Q')
            crypt = crypt + c.replace('=','C7A')
            crypt = crypt + c.replace("$",'D4E')
            crypt = crypt + c.replace('/','Q61')
            crypt = crypt + c.replace('<','M3T')
            crypt = crypt + c.replace('>','H6X')
            crypt = crypt + c.replace(';','E8I')
            crypt = crypt + c.replace(':','Y1U')
            crypt = crypt + c.replace('%','J3N')
            crypt = crypt + c.replace('^','P0Q')
        divide4count = 0
        for c in crypt:
            divide4count = divide4count + 1
            if divide4count%3.0 == 0:
                c2 = c + random.choice('1234567890')
                encrypted_text = encrypted_text + c2
            else:
                encrypted_text = encrypted_text + c
        self.entryVariable.set(encrypted_text[::-1])
        self.labelVariable.set("Encrypted")
        self.entry.selection_range(0, Tkinter.END)

The goal of this program is to make a simple secret code for privacy purposes. The output is supposed to be in this format: X1X1X1X1X1X1X1X1X1X1X1 Letter, Number, Letter, Random junk number

However, after entering a lowercase a as input, it encrypted it to aa9aaa1aaa6aaa0aaa5aaa8aaa9aaa8aaa6aaa6aaa3aaa9aaa4aaa9aaa4aaa3aaa3aaa6aaa4aaa7aQ85Paa3aaa2aaa6aaa5aaa7aaa9aaa0aaa5aaa

The code converts symbols (the alphabet and punctuation.) to alphanumeric strings of the format X1X, the adds a random number after those to make it a letter, number pattern.

Can anyone explain to me why I am getting a different output than the one I intended to get?Also could you please help me in fixing it?

Lastly, could anyone tell me whether this code is secure, and if not, how it could be broken, so I can improve it?

Please reduce this to a [minimal example](http://stackoverflow.com/help/mcve). And if you want genuine security, don't roll your own. — jonrsharpe, Aug 04 '14 at 20:49
it's gonna be fun when someone comes in with some Arabic, Japanese, etc. text and falls through your string replacments....as was stated, DO NOT ROLL YOUR OWN. Security standards exist for a reason. — user2366842, Aug 04 '14 at 21:00
Sorry for double comment, my last one was locked out from editing. You should look into something like a triple DES implementation instead. — user2366842, Aug 04 '14 at 21:07
@jonrsharpe I reduced it down to the area of the code that was giving me the unexpected result. — Bretsky, Aug 04 '14 at 21:18
@Bretsky much better, thanks. You would find it easier to debug if you separated the encryption from the display - a standalone function `def encrypt(s)` that returns the encrypted string means you can test the encryption without worrying about how it will be shown to the user. — jonrsharpe, Aug 04 '14 at 21:21
I'm not trying to make something real secure.It's just to practice my programming and once I get it working, I want to figure out how to crack it. — Bretsky, Aug 04 '14 at 21:26

score 3 · Accepted Answer · edited May 23 '17 at 12:11

Wow. Well, let's imagine the input text (which should not be so closely coupled to the GUI) is "C":

crypt = ""
for c in string: # c == 'C'                                 see what I did there?
    crypt = crypt + c.replace('A','F5K') # crypt = "C"      wait...
    crypt = crypt + c.replace('B','O7C') # crypt = "CC"     what?
    crypt = crypt + c.replace('C','E1S') # crypt = "CCE1S"  well that's ok
    crypt = crypt + c.replace('D','K1M') # crypt = "CCE1SC" oh but now...

That continues for another 60-odd harrowing lines. Then repeats for each character. Then, for some reason, you go back through inserting random numbers; why didn't you do that as each character was encoded?!

You should use a dictionary mapping e.g. {'A': 'F5K', ...}, then the encryption becomes much simpler:

import string 

...

crypt = [] # don't keep adding strings, they're immutable

for c in s: # don't call it string
    crypt.append(mapping.get(c, c))
    crypt.append(random.choice(string.digits)) # why do that all at the end?!

Notes:

I am using the string module, so am not using the name string for the string I'm iterating over. I could instead from string import digits, but string is such a frequently-used module that I tend to avoid that name anyway.
Strings are immutable, so "addition" really creates a new object, which is relatively inefficient. See e.g. here and here; str.join is preferred, particularly where you are adding a lot of strings (as you would be for a long message).

You can then just assign encrypted_text = "".join(crypt).

This should all be wrapped in a function that neither knows nor cares what the GUI is (or even if there is one).

I3L1N4Y2V1B9V1B4L2K2M5Z7S315D6V2L2K6A7S9V1B4Q2L1E7L4

Sorry for newbiness, but I do not understand what you did there apart from the dictionary mapping. — Bretsky, Aug 04 '14 at 22:14
You'll still want to be careful even with this solution though, because as I pointed out earlier, there's NUMEROUS unicode (and even a few that aren't) characters that can fall through your conditions. Right now, if you were to type in |||||||||| for example, it'd come out unencrypted. — user2366842, Aug 05 '14 at 00:25
@Bretsky I did barely anything other than the mapping; please review the first five sections of [the tutorial](https://docs.python.org/2/tutorial/) then ask a more specific question if you still have one. — jonrsharpe, Aug 05 '14 at 05:53
@user2366842 perfectly true, and as you have pointed out there are other, better options. — jonrsharpe, Aug 05 '14 at 05:54
@jonrsharpe why don't you call it string, and what is wrong with adding strings? — Bretsky, Aug 05 '14 at 08:45

Wrong output for substitution code in Python

1 Answers1

Linked

Related